[PATCH 00/17] leak fixes: use existing constructors & other trivia

[PATCH 00/17] leak fixes: use existing constructors & other trivia

[Ævar Arnfjörð Bjarmason]

With the very minor exceptions of:

* 03-04/17 (which need trivial oilerplate)
* 05/17 (need to add trivial control flow to a free_*() function)
* 12/17 (narrowing scope of allocation)
* 17/17: Add "goto ret" pattern, combine two "ret" variables

These are all "one-line" leak fixes where we merely need to make use
of an existing release function. The "one-line" only having the slight
disclaimer of needing to e.g. add braces to an "if" in one case, etc.

Each commit in this series is tested with:

	GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_SANITIZE_LEAK_LOG=true \
	make SANITIZE=leak test

I.e. mark tests as leak-free as we fix the leaks.

In 17/17 I replace uses of UNLEAK() where we can just as easily free()
instead, i.e. most of it's built-ins doing UNLEAK(x) instead of
strbuf_release(&x) etc.

As 17/17 notes I still think these's a place for unleak (some of the
remaining ones are quite tricky), but that we gain more from leaving
it for those tricky cases. Before this series we have 28 uses of
UNLEAK(), after it's 15.

Ævar Arnfjörð Bjarmason (17):
  tests: mark tests as passing with SANITIZE=leak
  {reset,merge}: call discard_index() before returning
  commit: discard partial cache before (re-)reading it
  read-cache.c: clear and free "sparse_checkout_patterns"
  dir.c: free "ident" and "exclude_per_dir" in "struct untracked_cache"
  built-ins & libs & helpers: add/move destructors, fix leaks
  unpack-file: fix ancient leak in create_temp_file()
  revision API: call graph_clear() in release_revisions()
  ls-files: fix a --with-tree memory leak
  sequencer.c: fix "opts->strategy" leak in read_strategy_opts()
  connected.c: free the "struct packed_git"
  sequencer.c: fix a pick_commits() leak
  rebase: don't leak on "--abort"
  sequencer.c: fix sequencer_continue() leak
  cherry-pick: free "struct replay_opts" members
  revert: fix parse_options_concat() leak
  built-ins: use free() not UNLEAK() if trivial, rm dead code

 builtin/add.c                            |  2 +-
 builtin/bugreport.c                      |  9 +++--
 builtin/checkout.c                       |  2 ++
 builtin/commit.c                         | 13 +++++---
 builtin/config.c                         | 42 +++++++++++-------------
 builtin/diff.c                           |  2 +-
 builtin/ls-files.c                       |  1 +
 builtin/merge.c                          |  1 +
 builtin/rebase.c                         |  4 +++
 builtin/repack.c                         |  2 +-
 builtin/reset.c                          |  2 ++
 builtin/rev-parse.c                      |  1 +
 builtin/revert.c                         |  4 +++
 builtin/stash.c                          |  2 ++
 builtin/unpack-file.c                    |  1 +
 builtin/worktree.c                       |  7 ++--
 connected.c                              |  6 +++-
 dir.c                                    | 10 ++++--
 dir.h                                    |  1 +
 read-cache.c                             |  5 +++
 ref-filter.c                             |  1 +
 revision.c                               |  1 +
 sequencer.c                              | 12 +++++--
 t/helper/test-fake-ssh.c                 |  1 +
 t/t0068-for-each-repo.sh                 |  1 +
 t/t0070-fundamental.sh                   |  1 +
 t/t1011-read-tree-sparse-checkout.sh     |  1 +
 t/t1022-read-tree-partial-clone.sh       |  2 +-
 t/t1404-update-ref-errors.sh             |  2 ++
 t/t1409-avoid-packing-refs.sh            |  1 +
 t/t1413-reflog-detach.sh                 |  1 +
 t/t1501-work-tree.sh                     |  2 ++
 t/t2012-checkout-last.sh                 |  1 +
 t/t2018-checkout-branch.sh               |  1 +
 t/t2025-checkout-no-overlay.sh           |  1 +
 t/t3009-ls-files-others-nonsubmodule.sh  |  1 +
 t/t3010-ls-files-killed-modified.sh      |  2 ++
 t/t3050-subprojects-fetch.sh             |  1 +
 t/t3060-ls-files-with-tree.sh            |  2 ++
 t/t3409-rebase-environ.sh                |  1 +
 t/t3413-rebase-hook.sh                   |  1 +
 t/t3428-rebase-signoff.sh                |  1 +
 t/t3429-rebase-edit-todo.sh              |  1 +
 t/t3433-rebase-across-mode-change.sh     |  1 +
 t/t4015-diff-whitespace.sh               |  4 +--
 t/t4045-diff-relative.sh                 |  2 ++
 t/t4052-stat-output.sh                   |  1 +
 t/t4053-diff-no-index.sh                 |  1 +
 t/t4067-diff-partial-clone.sh            |  1 +
 t/t4111-apply-subdir.sh                  |  1 +
 t/t4135-apply-weird-filenames.sh         |  1 +
 t/t4213-log-tabexpand.sh                 |  1 +
 t/t5544-pack-objects-hook.sh             |  2 ++
 t/t5554-noop-fetch-negotiator.sh         |  2 ++
 t/t5610-clone-detached.sh                |  1 +
 t/t5611-clone-config.sh                  |  1 +
 t/t5614-clone-submodules-shallow.sh      |  1 +
 t/t5617-clone-submodules-remote.sh       |  1 +
 t/t5618-alternate-refs.sh                |  2 ++
 t/t6060-merge-index.sh                   |  2 ++
 t/t6301-for-each-ref-errors.sh           |  1 +
 t/t6401-merge-criss-cross.sh             |  2 ++
 t/t6406-merge-attr.sh                    |  1 +
 t/t6407-merge-binary.sh                  |  1 +
 t/t6415-merge-dir-to-symlink.sh          |  1 +
 t/t6435-merge-sparse.sh                  |  1 +
 t/t7103-reset-bare.sh                    |  2 +-
 t/t7504-commit-msg-hook.sh               |  1 +
 t/t7517-per-repo-email.sh                |  1 +
 t/t7520-ignored-hook-warning.sh          |  1 +
 t/t7605-merge-resolve.sh                 |  1 +
 t/t7614-merge-signoff.sh                 |  1 +
 t/t9003-help-autocorrect.sh              |  2 ++
 t/t9115-git-svn-dcommit-funky-renames.sh |  1 -
 t/t9146-git-svn-empty-dirs.sh            |  1 -
 t/t9148-git-svn-propset.sh               |  1 -
 t/t9160-git-svn-preserve-empty-dirs.sh   |  1 -
 77 files changed, 150 insertions(+), 51 deletions(-)

-- 
2.38.0.1451.g86b35f4140a

[PATCH 01/17] tests: mark tests as passing with SANITIZE=leak

[Ævar Arnfjörð Bjarmason]

This marks tests that have been leak-free since various recent
commits, but which were not marked us such when the memory leak was
fixed. These were mostly discovered with the "check" mode added in
faececa53f9 (test-lib: have the "check" mode for SANITIZE=leak
consider leak logs, 2022-07-28).

Commits that fixed the last memory leak in these tests. Per narrowing
down when they started to pass under SANITIZE=leak with "bisect":

- t1022-read-tree-partial-clone.sh:
  7e2619d8ff0 (list_objects_filter_options: plug leak of filter_spec
  strings, 2022-09-08)

- t4053-diff-no-index.sh: 07a6f94a6d0 (diff-no-index: release prefixed
  filenames, 2022-09-07)

- t6415-merge-dir-to-symlink.sh: bac92b1f39f (Merge branch
  'js/ort-clean-up-after-failed-merge', 2022-08-08).

- t5554-noop-fetch-negotiator.sh:
  66eede4a37c (prepare_repo_settings(): plug leak of config values,
  2022-09-08)

Let's mark all of these as passing with
"TEST_PASSES_SANITIZE_LEAK=true", to have it regression tested,
including as part of the "linux-leaks" CI job.

Additionally, let's remove the "!SANITIZE_LEAK" prerequisite from
tests that now pass, these were marked as failing in:

- 77e56d55ba6 (diff.c: fix a double-free regression in a18d66cefb,
  2022-03-17)
- c4d1d526312 (tests: change some 'test $(git) = "x"' to test_cmp,
  2022-03-07)

These were not spotted with the new "check" mode, but manually, it
doesn't cover these sort of prerequisites. There's few enough that we
shouldn't bother to automate it. They'll be going away sooner than
later.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1022-read-tree-partial-clone.sh | 2 +-
 t/t4015-diff-whitespace.sh         | 4 ++--
 t/t4053-diff-no-index.sh           | 1 +
 t/t5554-noop-fetch-negotiator.sh   | 2 ++
 t/t6415-merge-dir-to-symlink.sh    | 1 +
 t/t7103-reset-bare.sh              | 2 +-
 6 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/t/t1022-read-tree-partial-clone.sh b/t/t1022-read-tree-partial-clone.sh
index a9953b6a71c..d08563de5c9 100755
--- a/t/t1022-read-tree-partial-clone.sh
+++ b/t/t1022-read-tree-partial-clone.sh
@@ -3,7 +3,7 @@
 test_description='git read-tree in partial clones'
 
 TEST_NO_CREATE_REPO=1
-
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'read-tree in partial clone prefetches in one batch' '
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index f3e20dd5bba..b298f220e01 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -1638,7 +1638,7 @@ test_expect_success 'no effect on diff from --color-moved with --word-diff' '
 	test_cmp expect actual
 '
 
-test_expect_success !SANITIZE_LEAK 'no effect on show from --color-moved with --word-diff' '
+test_expect_success 'no effect on show from --color-moved with --word-diff' '
 	git show --color-moved --word-diff >actual &&
 	git show --word-diff >expect &&
 	test_cmp expect actual
@@ -2024,7 +2024,7 @@ test_expect_success '--color-moved rewinds for MIN_ALNUM_COUNT' '
 	test_cmp expected actual
 '
 
-test_expect_success !SANITIZE_LEAK 'move detection with submodules' '
+test_expect_success 'move detection with submodules' '
 	test_create_repo bananas &&
 	echo ripe >bananas/recipe &&
 	git -C bananas add recipe &&
diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh
index 3feadf0e359..4e9fa0403d3 100755
--- a/t/t4053-diff-no-index.sh
+++ b/t/t4053-diff-no-index.sh
@@ -2,6 +2,7 @@
 
 test_description='diff --no-index'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t5554-noop-fetch-negotiator.sh b/t/t5554-noop-fetch-negotiator.sh
index 2ac7b5859e7..06991e8e8aa 100755
--- a/t/t5554-noop-fetch-negotiator.sh
+++ b/t/t5554-noop-fetch-negotiator.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='test noop fetch negotiator'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'noop negotiator does not emit any "have"' '
diff --git a/t/t6415-merge-dir-to-symlink.sh b/t/t6415-merge-dir-to-symlink.sh
index 2655e295f5a..ae00492c768 100755
--- a/t/t6415-merge-dir-to-symlink.sh
+++ b/t/t6415-merge-dir-to-symlink.sh
@@ -4,6 +4,7 @@ test_description='merging when a directory was replaced with a symlink'
 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 'create a commit where dir a/b changed to symlink' '
diff --git a/t/t7103-reset-bare.sh b/t/t7103-reset-bare.sh
index a60153f9f32..18bbd9975eb 100755
--- a/t/t7103-reset-bare.sh
+++ b/t/t7103-reset-bare.sh
@@ -63,7 +63,7 @@ test_expect_success '"mixed" reset is not allowed in bare' '
 	test_must_fail git reset --mixed HEAD^
 '
 
-test_expect_success !SANITIZE_LEAK '"soft" reset is allowed in bare' '
+test_expect_success '"soft" reset is allowed in bare' '
 	git reset --soft HEAD^ &&
 	git show --pretty=format:%s >out &&
 	echo one >expect &&
-- 
2.38.0.1451.g86b35f4140a

[PATCH 02/17] {reset,merge}: call discard_index() before returning

[Ævar Arnfjörð Bjarmason]

These two built-ins both deal with the index, but weren't discarding
it. In subsequent commits we'll add more free()-ing to discard_index()
that we've missed, but let's first call the existing function.

We can doubtless add discard_index() (or its alias discard_cache()) to
a lot more places, but let's just add it here for now.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/merge.c | 1 +
 builtin/reset.c | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/builtin/merge.c b/builtin/merge.c
index 5900b81729d..a6698adbfd3 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1794,5 +1794,6 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	}
 	strbuf_release(&buf);
 	free(branch_to_free);
+	discard_index(&the_index);
 	return ret;
 }
diff --git a/builtin/reset.c b/builtin/reset.c
index fdce6f8c856..69f18a248ee 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -481,5 +481,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	if (!pathspec.nr)
 		remove_branch_state(the_repository, 0);
 
+	discard_index(&the_index);
+
 	return update_ref_status;
 }
-- 
2.38.0.1451.g86b35f4140a

[PATCH 03/17] commit: discard partial cache before (re-)reading it

[Ævar Arnfjörð Bjarmason]

The read_cache() in prepare_to_commit() would end up clobbering the
pointer we had for a previously populated "the_index.cache_tree" in
the very common case of "git commit" stressed by e.g. the tests being
changed here.

We'd populate "the_index.cache_tree" by calling
"update_main_cache_tree" in prepare_index(), but would not end up with
a "fully prepared" index. What constitutes an existing index is
clearly overly fuzzy, here we'll check "active_nr" (aka
"the_index.cache_nr"), but our "the_index.cache_tree" might have been
malloc()'d already.

Thus the code added in 11c8a74a64a (commit: write cache-tree data when
writing index anyway, 2011-12-06) would end up allocating the
"cache_tree", and would interact here with code added in
7168624c353 (Do not generate full commit log message if it is not
going to be used, 2007-11-28). The result was a very common memory
leak.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/commit.c                        | 7 +++++--
 t/t0068-for-each-repo.sh                | 1 +
 t/t0070-fundamental.sh                  | 1 +
 t/t1404-update-ref-errors.sh            | 2 ++
 t/t1409-avoid-packing-refs.sh           | 1 +
 t/t1413-reflog-detach.sh                | 1 +
 t/t1501-work-tree.sh                    | 2 ++
 t/t2025-checkout-no-overlay.sh          | 1 +
 t/t3009-ls-files-others-nonsubmodule.sh | 1 +
 t/t3010-ls-files-killed-modified.sh     | 2 ++
 t/t4045-diff-relative.sh                | 2 ++
 t/t4111-apply-subdir.sh                 | 1 +
 t/t4135-apply-weird-filenames.sh        | 1 +
 t/t4213-log-tabexpand.sh                | 1 +
 t/t5618-alternate-refs.sh               | 2 ++
 t/t6301-for-each-ref-errors.sh          | 1 +
 t/t7520-ignored-hook-warning.sh         | 1 +
 t/t7614-merge-signoff.sh                | 1 +
 t/t9003-help-autocorrect.sh             | 2 ++
 19 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index e22bdf23f5f..c291199b704 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -987,8 +987,11 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		struct object_id oid;
 		const char *parent = "HEAD";
 
-		if (!active_nr && read_cache() < 0)
-			die(_("Cannot read index"));
+		if (!active_nr) {
+			discard_cache();
+			if (read_cache() < 0)
+				die(_("Cannot read index"));
+		}
 
 		if (amend)
 			parent = "HEAD^1";
diff --git a/t/t0068-for-each-repo.sh b/t/t0068-for-each-repo.sh
index 4675e852517..9f63f612425 100755
--- a/t/t0068-for-each-repo.sh
+++ b/t/t0068-for-each-repo.sh
@@ -2,6 +2,7 @@
 
 test_description='git for-each-repo builtin'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'run based on configured value' '
diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index 8d59905ef09..574de341980 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -6,6 +6,7 @@ test_description='check that the most basic functions work
 Verify wrappers and compatibility functions.
 '
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'character classes (isspace, isalpha etc.)' '
diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh
index 13c2b43bbae..b5606d93b52 100755
--- a/t/t1404-update-ref-errors.sh
+++ b/t/t1404-update-ref-errors.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='Test git update-ref error handling'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Create some references, perhaps run pack-refs --all, then try to
diff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh
index be12fb63506..f23c0152a85 100755
--- a/t/t1409-avoid-packing-refs.sh
+++ b/t/t1409-avoid-packing-refs.sh
@@ -2,6 +2,7 @@
 
 test_description='avoid rewriting packed-refs unnecessarily'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Add an identifying mark to the packed-refs file header line. This
diff --git a/t/t1413-reflog-detach.sh b/t/t1413-reflog-detach.sh
index 934688a1ee8..d2a4822d46f 100755
--- a/t/t1413-reflog-detach.sh
+++ b/t/t1413-reflog-detach.sh
@@ -4,6 +4,7 @@ test_description='Test reflog interaction with detached HEAD'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 reset_state () {
diff --git a/t/t1501-work-tree.sh b/t/t1501-work-tree.sh
index b75558040ff..ae6528aecea 100755
--- a/t/t1501-work-tree.sh
+++ b/t/t1501-work-tree.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='test separate work tree'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t2025-checkout-no-overlay.sh b/t/t2025-checkout-no-overlay.sh
index 8f13341cf8e..3832c3de813 100755
--- a/t/t2025-checkout-no-overlay.sh
+++ b/t/t2025-checkout-no-overlay.sh
@@ -2,6 +2,7 @@
 
 test_description='checkout --no-overlay <tree-ish> -- <pathspec>'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t3009-ls-files-others-nonsubmodule.sh b/t/t3009-ls-files-others-nonsubmodule.sh
index 963f3462b75..14218b34243 100755
--- a/t/t3009-ls-files-others-nonsubmodule.sh
+++ b/t/t3009-ls-files-others-nonsubmodule.sh
@@ -18,6 +18,7 @@ This test runs git ls-files --others with the following working tree:
       git repository with a commit and an untracked file
 '
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup: directories' '
diff --git a/t/t3010-ls-files-killed-modified.sh b/t/t3010-ls-files-killed-modified.sh
index 580e158f991..054178703d7 100755
--- a/t/t3010-ls-files-killed-modified.sh
+++ b/t/t3010-ls-files-killed-modified.sh
@@ -41,6 +41,8 @@ Also for modification test, the cache and working tree have:
 We should report path0, path1, path2/file2, path3/file3, path7 and path8
 modified without reporting path9 and path10.  submod1 is also modified.
 '
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'git update-index --add to add various paths.' '
diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh
index fab351b48a1..198dfc91906 100755
--- a/t/t4045-diff-relative.sh
+++ b/t/t4045-diff-relative.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='diff --relative tests'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t4111-apply-subdir.sh b/t/t4111-apply-subdir.sh
index 1618a6dbc7c..e9a87d761d1 100755
--- a/t/t4111-apply-subdir.sh
+++ b/t/t4111-apply-subdir.sh
@@ -2,6 +2,7 @@
 
 test_description='patching from inconvenient places'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t4135-apply-weird-filenames.sh b/t/t4135-apply-weird-filenames.sh
index 6bc3fb97a75..d3502c6fddf 100755
--- a/t/t4135-apply-weird-filenames.sh
+++ b/t/t4135-apply-weird-filenames.sh
@@ -2,6 +2,7 @@
 
 test_description='git apply with weird postimage filenames'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t4213-log-tabexpand.sh b/t/t4213-log-tabexpand.sh
index 53a4af32449..590fce95e90 100755
--- a/t/t4213-log-tabexpand.sh
+++ b/t/t4213-log-tabexpand.sh
@@ -2,6 +2,7 @@
 
 test_description='log/show --expand-tabs'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 HT="	"
diff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh
index 3353216f09e..f905db0a3fd 100755
--- a/t/t5618-alternate-refs.sh
+++ b/t/t5618-alternate-refs.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='test handling of --alternate-refs traversal'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Avoid test_commit because we want a specific and known set of refs:
diff --git a/t/t6301-for-each-ref-errors.sh b/t/t6301-for-each-ref-errors.sh
index 40edf9dab53..bfda1f46ad2 100755
--- a/t/t6301-for-each-ref-errors.sh
+++ b/t/t6301-for-each-ref-errors.sh
@@ -2,6 +2,7 @@
 
 test_description='for-each-ref errors for broken refs'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 ZEROS=$ZERO_OID
diff --git a/t/t7520-ignored-hook-warning.sh b/t/t7520-ignored-hook-warning.sh
index dc57526e6f1..184b2589893 100755
--- a/t/t7520-ignored-hook-warning.sh
+++ b/t/t7520-ignored-hook-warning.sh
@@ -2,6 +2,7 @@
 
 test_description='ignored hook warning'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '
diff --git a/t/t7614-merge-signoff.sh b/t/t7614-merge-signoff.sh
index fee258d4f0c..cf96a35e8e7 100755
--- a/t/t7614-merge-signoff.sh
+++ b/t/t7614-merge-signoff.sh
@@ -8,6 +8,7 @@ This test runs git merge --signoff and makes sure that it works.
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Setup test files
diff --git a/t/t9003-help-autocorrect.sh b/t/t9003-help-autocorrect.sh
index f00deaf3815..4b9cb4c942f 100755
--- a/t/t9003-help-autocorrect.sh
+++ b/t/t9003-help-autocorrect.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='help.autocorrect finding a match'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-- 
2.38.0.1451.g86b35f4140a

[PATCH 04/17] read-cache.c: clear and free "sparse_checkout_patterns"

[Ævar Arnfjörð Bjarmason]

The "sparse_checkout_patterns" member was added to the "struct
index_state" in 836e25c51b2 (sparse-checkout: hold pattern list in
index, 2021-03-30), but wasn't added to discard_index(). Let's do
that.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 read-cache.c                         | 5 +++++
 t/t1011-read-tree-sparse-checkout.sh | 1 +
 t/t2018-checkout-branch.sh           | 1 +
 t/t6435-merge-sparse.sh              | 1 +
 4 files changed, 8 insertions(+)

diff --git a/read-cache.c b/read-cache.c
index 32024029274..7c6477487a7 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2558,6 +2558,11 @@ int discard_index(struct index_state *istate)
 	free_untracked_cache(istate->untracked);
 	istate->untracked = NULL;
 
+	if (istate->sparse_checkout_patterns) {
+		clear_pattern_list(istate->sparse_checkout_patterns);
+		FREE_AND_NULL(istate->sparse_checkout_patterns);
+	}
+
 	if (istate->ce_mem_pool) {
 		mem_pool_discard(istate->ce_mem_pool, should_validate_cache_entries());
 		FREE_AND_NULL(istate->ce_mem_pool);
diff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh
index 742f0fa909f..595b24c0adb 100755
--- a/t/t1011-read-tree-sparse-checkout.sh
+++ b/t/t1011-read-tree-sparse-checkout.sh
@@ -12,6 +12,7 @@ test_description='sparse checkout tests
 '
 
 TEST_CREATE_REPO_NO_TEMPLATE=1
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-read-tree.sh
 
diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
index 771c3c3c50e..8581ad34379 100755
--- a/t/t2018-checkout-branch.sh
+++ b/t/t2018-checkout-branch.sh
@@ -3,6 +3,7 @@
 test_description='checkout'
 
 TEST_CREATE_REPO_NO_TEMPLATE=1
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Arguments: [!] <branch> <oid> [<checkout options>]
diff --git a/t/t6435-merge-sparse.sh b/t/t6435-merge-sparse.sh
index fde4aa3cd1a..78628fb248a 100755
--- a/t/t6435-merge-sparse.sh
+++ b/t/t6435-merge-sparse.sh
@@ -3,6 +3,7 @@
 test_description='merge with sparse files'
 
 TEST_CREATE_REPO_NO_TEMPLATE=1
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # test_file $filename $content
-- 
2.38.0.1451.g86b35f4140a

[PATCH 05/17] dir.c: free "ident" and "exclude_per_dir" in "struct untracked_cache"

[Ævar Arnfjörð Bjarmason]

When the "ident" member of the structure was added in
1e8fef609e7 (untracked cache: guard and disable on system changes,
2015-03-08) this function wasn't updated to free it. Let's do so.

Let's also free the "exclude_per_dir" memory we've been leaking
since[1], while making sure not to free() the constant ".gitignore"
string we add by default[2].

As we now have three struct members we're freeing let's change
free_untracked_cache() to return early if "uc" isn't defined. We won't
hand it to free() now, but that was just for convenience, once we're
dealing with >=2 struct members this pattern is more convenient.

1. f9e6c649589 (untracked cache: load from UNTR index extension,
   2015-03-08)
2. 039bc64e886 (core.excludesfile clean-up, 2007-11-14)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 dir.c | 10 +++++++---
 dir.h |  1 +
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/dir.c b/dir.c
index d604d1bab98..fbdb24fc819 100644
--- a/dir.c
+++ b/dir.c
@@ -3581,8 +3581,12 @@ static void free_untracked(struct untracked_cache_dir *ucd)
 
 void free_untracked_cache(struct untracked_cache *uc)
 {
-	if (uc)
-		free_untracked(uc->root);
+	if (!uc)
+		return;
+
+	free(uc->exclude_per_dir_to_free);
+	strbuf_release(&uc->ident);
+	free_untracked(uc->root);
 	free(uc);
 }
 
@@ -3739,7 +3743,7 @@ struct untracked_cache *read_untracked_extension(const void *data, unsigned long
 		      next + offset + hashsz);
 	uc->dir_flags = get_be32(next + ouc_offset(dir_flags));
 	exclude_per_dir = (const char *)next + exclude_per_dir_offset;
-	uc->exclude_per_dir = xstrdup(exclude_per_dir);
+	uc->exclude_per_dir = uc->exclude_per_dir_to_free = xstrdup(exclude_per_dir);
 	/* NUL after exclude_per_dir is covered by sizeof(*ouc) */
 	next += exclude_per_dir_offset + strlen(exclude_per_dir) + 1;
 	if (next >= end)
diff --git a/dir.h b/dir.h
index 674747d93af..8acfc044181 100644
--- a/dir.h
+++ b/dir.h
@@ -188,6 +188,7 @@ struct untracked_cache {
 	struct oid_stat ss_info_exclude;
 	struct oid_stat ss_excludes_file;
 	const char *exclude_per_dir;
+	char *exclude_per_dir_to_free;
 	struct strbuf ident;
 	/*
 	 * dir_struct#flags must match dir_flags or the untracked
-- 
2.38.0.1451.g86b35f4140a

[PATCH 06/17] built-ins & libs & helpers: add/move destructors, fix leaks

[Ævar Arnfjörð Bjarmason]

Fix various leaks in built-ins, libraries and a test helper here we
were missing a call to strbuf_release(), string_list_clear() etc, or
were calling them after a potential "return".

Comments on individual changes:

- builtin/checkout.c: Fix a memory leak that was introduced in [1]. A
  sibling leak introduced in [2] was recently fixed in [3]. As with [3]
  we should be using the wt_status_state_free_buffers() API introduced
  in [4].

- builtin/repack.c: Fix a leak that's been here since this use of
  "strbuf_release()" was added in a1bbc6c0176 (repack: rewrite the shell
  script in C, 2013-09-15). We don't use the variable for anything
  except this loop, so we can instead free it right afterwards.

- builtin/rev-parse: Fix a leak that's been here since this code was
  added in 21d47835386 (Add a parseopt mode to git-rev-parse to bring
  parse-options to shell scripts., 2007-11-04).

- builtin/stash.c: Fix a couple of leaks that have been here since
  this code was added in d4788af875c (stash: convert create to builtin,
  2019-02-25), we strbuf_release()'d only some of the "struct strbuf" we
  allocated earlier in the function, let's release all of them.

- ref-filter.c: Fix a leak in 482c1191869 (gpg-interface: improve
  interface for parsing tags, 2021-02-11), we don't use the "payload"
  variable that we ask parse_signature() to populate for us, so let's
  free it.

- t/helper/test-fake-ssh.c: Fix a leak that's been here since this
  code was added in 3064d5a38c7 (mingw: fix t5601-clone.sh,
  2016-01-27). Let's free the "struct strbuf" as soon as we don't need
  it anymore.

1. c45f0f525de (switch: reject if some operation is in progress,
   2019-03-29)
2. 2708ce62d21 (branch: sort detached HEAD based on a flag,
   2021-01-07)
3. abcac2e19fa (ref-filter.c: fix a leak in get_head_description,
   2022-09-25)
4. 962dd7ebc3e (wt-status: introduce wt_status_state_free_buffers(),
   2020-09-27).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/checkout.c        | 2 ++
 builtin/rebase.c          | 3 +++
 builtin/repack.c          | 2 +-
 builtin/rev-parse.c       | 1 +
 builtin/stash.c           | 2 ++
 ref-filter.c              | 1 +
 t/helper/test-fake-ssh.c  | 1 +
 t/t3428-rebase-signoff.sh | 1 +
 8 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 2a132392fbe..659dd5c4309 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1470,6 +1470,8 @@ static void die_if_some_operation_in_progress(void)
 		      "or \"git worktree add\"."));
 	if (state.bisect_in_progress)
 		warning(_("you are switching branch while bisecting"));
+
+	wt_status_state_free_buffers(&state);
 }
 
 static int checkout_branch(struct checkout_opts *opts,
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 5d855fd8f51..64aed11c85d 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1825,10 +1825,13 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	strbuf_release(&buf);
 	strbuf_release(&revisions);
 	free(options.head_name);
+	strvec_clear(&options.git_am_opts);
 	free(options.gpg_sign_opt);
 	free(options.cmd);
 	free(options.strategy);
 	strbuf_release(&options.git_format_patch_opt);
 	free(squash_onto_name);
+	string_list_clear(&exec, 0);
+	string_list_clear(&strategy_options, 0);
 	return !!ret;
 }
diff --git a/builtin/repack.c b/builtin/repack.c
index 10e23f9ee1f..fb3ac197426 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -956,6 +956,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 		item = string_list_append(&names, line.buf);
 		item->util = populate_pack_exts(item->string);
 	}
+	strbuf_release(&line);
 	fclose(out);
 	ret = finish_command(&cmd);
 	if (ret)
@@ -1124,7 +1125,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	string_list_clear(&existing_nonkept_packs, 0);
 	string_list_clear(&existing_kept_packs, 0);
 	clear_pack_geometry(geometry);
-	strbuf_release(&line);
 
 	return 0;
 }
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 8f61050bde8..e0244d63de6 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -530,6 +530,7 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)
 	strbuf_addstr(&parsed, " --");
 	sq_quote_argv(&parsed, argv);
 	puts(parsed.buf);
+	strbuf_release(&parsed);
 	return 0;
 }
 
diff --git a/builtin/stash.c b/builtin/stash.c
index bb5485b4095..8a64d564a19 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1686,8 +1686,10 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
 	}
 
 done:
+	strbuf_release(&patch);
 	free_stash_info(&info);
 	strbuf_release(&stash_msg_buf);
+	strbuf_release(&untracked_files);
 	return ret;
 }
 
diff --git a/ref-filter.c b/ref-filter.c
index 914908fac52..b40b76c3806 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1358,6 +1358,7 @@ static void find_subpos(const char *buf,
 
 	/* parse signature first; we might not even have a subject line */
 	parse_signature(buf, end - buf, &payload, &signature);
+	strbuf_release(&payload);
 
 	/* skip past header until we hit empty line */
 	while (*buf && *buf != '\n') {
diff --git a/t/helper/test-fake-ssh.c b/t/helper/test-fake-ssh.c
index 12beee99ad2..d5e511633ab 100644
--- a/t/helper/test-fake-ssh.c
+++ b/t/helper/test-fake-ssh.c
@@ -17,6 +17,7 @@ int cmd_main(int argc, const char **argv)
 	f = fopen(buf.buf, "w");
 	if (!f)
 		die("Could not write to %s", buf.buf);
+	strbuf_release(&buf);
 	for (i = 0; i < argc; i++)
 		fprintf(f, "%s%s", i > 0 ? " " : "", i > 0 ? argv[i] : "ssh:");
 	fprintf(f, "\n");
diff --git a/t/t3428-rebase-signoff.sh b/t/t3428-rebase-signoff.sh
index f6993b7e14d..e1b1e947647 100755
--- a/t/t3428-rebase-signoff.sh
+++ b/t/t3428-rebase-signoff.sh
@@ -5,6 +5,7 @@ test_description='git rebase --signoff
 This test runs git rebase --signoff and make sure that it works.
 '
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # A simple file to commit
-- 
2.38.0.1451.g86b35f4140a

[PATCH 07/17] unpack-file: fix ancient leak in create_temp_file()

[Ævar Arnfjörð Bjarmason]

Fix a leak that's been with us since 3407bb4940c (Add "unpack-file"
helper that unpacks a sha1 blob into a tmpfile., 2005-04-18). See
00c8fd493af (cat-file: use streaming API to print blobs, 2012-03-07)
for prior art which shows the same API pattern, i.e. free()-ing the
result of read_object_file() after it's used.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/unpack-file.c        | 1 +
 t/t6060-merge-index.sh       | 2 ++
 t/t6401-merge-criss-cross.sh | 2 ++
 t/t6406-merge-attr.sh        | 1 +
 t/t6407-merge-binary.sh      | 1 +
 t/t7605-merge-resolve.sh     | 1 +
 6 files changed, 8 insertions(+)

diff --git a/builtin/unpack-file.c b/builtin/unpack-file.c
index 9e8119dd354..88de32b7d7e 100644
--- a/builtin/unpack-file.c
+++ b/builtin/unpack-file.c
@@ -19,6 +19,7 @@ static char *create_temp_file(struct object_id *oid)
 	if (write_in_full(fd, buf, size) < 0)
 		die_errno("unable to write temp-file");
 	close(fd);
+	free(buf);
 	return path;
 }
 
diff --git a/t/t6060-merge-index.sh b/t/t6060-merge-index.sh
index ed449abe552..1a8b64cce18 100755
--- a/t/t6060-merge-index.sh
+++ b/t/t6060-merge-index.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='basic git merge-index / git-merge-one-file tests'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup diverging branches' '
diff --git a/t/t6401-merge-criss-cross.sh b/t/t6401-merge-criss-cross.sh
index 9d5e992878f..1962310408b 100755
--- a/t/t6401-merge-criss-cross.sh
+++ b/t/t6401-merge-criss-cross.sh
@@ -8,6 +8,8 @@
 
 
 test_description='Test criss-cross merge'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'prepare repository' '
diff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh
index 8650a88c40a..5e4e4dd6d9e 100755
--- a/t/t6406-merge-attr.sh
+++ b/t/t6406-merge-attr.sh
@@ -8,6 +8,7 @@ test_description='per path merge controlled by merge attribute'
 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 '
diff --git a/t/t6407-merge-binary.sh b/t/t6407-merge-binary.sh
index e8a28717cec..0753fc95f45 100755
--- a/t/t6407-merge-binary.sh
+++ b/t/t6407-merge-binary.sh
@@ -5,6 +5,7 @@ test_description='ask merge-recursive to merge binary files'
 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 '
diff --git a/t/t7605-merge-resolve.sh b/t/t7605-merge-resolve.sh
index 5d56c385464..62d935d31c2 100755
--- a/t/t7605-merge-resolve.sh
+++ b/t/t7605-merge-resolve.sh
@@ -4,6 +4,7 @@ test_description='git merge
 
 Testing the resolve strategy.'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-- 
2.38.0.1451.g86b35f4140a

[PATCH 08/17] revision API: call graph_clear() in release_revisions()

[Ævar Arnfjörð Bjarmason]

Call graph_clear() in release_revisions(), this will free memory
allocated by e.g. this command, which will now run without memory
leaks:

	git -P log -1 --graph --no-graph --graph

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 revision.c             | 1 +
 t/t4052-stat-output.sh | 1 +
 2 files changed, 2 insertions(+)

diff --git a/revision.c b/revision.c
index 0760e78936e..650dd17599a 100644
--- a/revision.c
+++ b/revision.c
@@ -3020,6 +3020,7 @@ void release_revisions(struct rev_info *revs)
 	date_mode_release(&revs->date_mode);
 	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->pruning);
 	reflog_walk_info_release(revs->reflog_info);
diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh
index b5c281edaa7..3ee27e277dc 100755
--- a/t/t4052-stat-output.sh
+++ b/t/t4052-stat-output.sh
@@ -8,6 +8,7 @@ test_description='test --stat output of various commands'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-terminal.sh
 
-- 
2.38.0.1451.g86b35f4140a

[PATCH 09/17] ls-files: fix a --with-tree memory leak

[Ævar Arnfjörð Bjarmason]

Fix a memory leak in overlay_tree_on_index(), we need to
clear_pathspec() at some point, which might as well be after the last
time we use it in the function.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/ls-files.c            | 1 +
 t/t3060-ls-files-with-tree.sh | 2 ++
 t/t9148-git-svn-propset.sh    | 1 -
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 4cf8a236483..a03b559ecaa 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -613,6 +613,7 @@ void overlay_tree_on_index(struct index_state *istate,
 	if (!fn)
 		fn = read_one_entry_quick;
 	err = read_tree(the_repository, tree, &pathspec, fn, istate);
+	clear_pathspec(&pathspec);
 	if (err)
 		die("unable to read tree entries %s", tree_name);
 
diff --git a/t/t3060-ls-files-with-tree.sh b/t/t3060-ls-files-with-tree.sh
index 52f76f7b57f..c4a72ae4462 100755
--- a/t/t3060-ls-files-with-tree.sh
+++ b/t/t3060-ls-files-with-tree.sh
@@ -8,6 +8,8 @@ test_description='git ls-files test (--with-tree).
 This test runs git ls-files --with-tree and in particular in
 a scenario known to trigger a crash with some versions of git.
 '
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t9148-git-svn-propset.sh b/t/t9148-git-svn-propset.sh
index 6cc76a07b39..aebb28995e5 100755
--- a/t/t9148-git-svn-propset.sh
+++ b/t/t9148-git-svn-propset.sh
@@ -5,7 +5,6 @@
 
 test_description='git svn propset tests'
 
-TEST_FAILS_SANITIZE_LEAK=true
 . ./lib-git-svn.sh
 
 test_expect_success 'setup propset via import' '
-- 
2.38.0.1451.g86b35f4140a

[PATCH 10/17] sequencer.c: fix "opts->strategy" leak in read_strategy_opts()

[Ævar Arnfjörð Bjarmason]

When "read_strategy_opts()" is called we may have populated the
"opts->strategy" before, so we'll need to free() it to avoid leaking
memory. Along with the preceding commit this change various
rebase-related tests pass.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 sequencer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sequencer.c b/sequencer.c
index e658df7e8ff..07d7062bfb8 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2894,6 +2894,7 @@ static void read_strategy_opts(struct replay_opts *opts, struct strbuf *buf)
 	strbuf_reset(buf);
 	if (!read_oneliner(buf, rebase_path_strategy(), 0))
 		return;
+	free(opts->strategy);
 	opts->strategy = strbuf_detach(buf, NULL);
 	if (!read_oneliner(buf, rebase_path_strategy_opts(), 0))
 		return;
-- 
2.38.0.1451.g86b35f4140a

Re: [PATCH 10/17] sequencer.c: fix "opts->strategy" leak in read_strategy_opts()

[Phillip Wood]

On 03/11/2022 17:06, Ævar Arnfjörð Bjarmason wrote:
> When "read_strategy_opts()" is called we may have populated the
> "opts->strategy" before, so we'll need to free() it to avoid leaking
> memory. 

Where is the previous value coming from? I guess it may be the config 
but otherwise I'm confused.

> Along with the preceding commit this change various
> rebase-related tests pass.

Really? at a glance the previous patch looks unrelated and there are no 
tests marked as passing in this one.

Best Wishes

Phillip

> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>   sequencer.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/sequencer.c b/sequencer.c
> index e658df7e8ff..07d7062bfb8 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2894,6 +2894,7 @@ static void read_strategy_opts(struct replay_opts *opts, struct strbuf *buf)
>   	strbuf_reset(buf);
>   	if (!read_oneliner(buf, rebase_path_strategy(), 0))
>   		return;
> +	free(opts->strategy);
>   	opts->strategy = strbuf_detach(buf, NULL);
>   	if (!read_oneliner(buf, rebase_path_strategy_opts(), 0))
>   		return;

Re: [PATCH 10/17] sequencer.c: fix "opts->strategy" leak in read_strategy_opts()

[Taylor Blau]

On Fri, Nov 04, 2022 at 02:50:02PM +0000, Phillip Wood wrote:
> > Along with the preceding commit this change various
> > rebase-related tests pass.
>
> Really? at a glance the previous patch looks unrelated and there are no
> tests marked as passing in this one.

Yeah, I wondered the same thing. What are we missing here?

Thanks,
Taylor

Re: [PATCH 10/17] sequencer.c: fix "opts->strategy" leak in read_strategy_opts()

[Ævar Arnfjörð Bjarmason]

On Fri, Nov 04 2022, Taylor Blau wrote:

> On Fri, Nov 04, 2022 at 02:50:02PM +0000, Phillip Wood wrote:
>> > Along with the preceding commit this change various
>> > rebase-related tests pass.
>>
>> Really? at a glance the previous patch looks unrelated and there are no
>> tests marked as passing in this one.
>
> Yeah, I wondered the same thing. What are we missing here?

I see I was using "tests that pass" in an arguably ambiguous way.

This does make more tests pass, but not in such a way that the entire
set of tests in any given file in t/t[0-9]*.sh now pases in its
entirety, which is the point at which we can add:

	TEST_PASSES_SANITIZE_LEAK=true

Re: [PATCH 10/17] sequencer.c: fix "opts->strategy" leak in read_strategy_opts()

[Phillip Wood]

On 04/11/2022 14:50, Phillip Wood wrote:
> On 03/11/2022 17:06, Ævar Arnfjörð Bjarmason wrote:
>> When "read_strategy_opts()" is called we may have populated the
>> "opts->strategy" before, so we'll need to free() it to avoid leaking
>> memory. 
> 
> Where is the previous value coming from? I guess it may be the config 
> but otherwise I'm confused.

Having looked a bit more at this I think this is the wrong fix. The 
reason we're overwriting the existing value is that we're reading some 
of the state files twice. I think the only way to get to this point is 
to go through a code path that calls rebase.c:read_basic_state() which 
already populates these options via a later call to 
rebase.c:get_replay_opts(). I think the correct fixes looks something 
like the diff below.

I have also looked at the cherry-pick/revert case and I think that we're 
leaking opts->strategy (and probably some others) when running

	git cherry-pick --continue

after

	git -c pull.twohead=recursive cherry-pick -s ort <some commits>

I'm not sure what your strategy has been with the fixes in this series 
but we're never going to have 100% coverage of all the option 
combinations for rebase & cherry-pick so I think it is helpful to treat 
these LSAN reports as a starting point for looking into why the leak is 
occurring and also look for similar leaks.

Best Wishes

Phillip

--- >8 ---
  sequencer.c | 46 ----------------------------------------------
  1 file changed, 46 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index e658df7e8f..ece34dce9f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2889,55 +2889,12 @@ void parse_strategy_opts(struct replay_opts 
*opts, char *raw_opts)
         }
  }

-static void read_strategy_opts(struct replay_opts *opts, struct strbuf 
*buf)
-{
-       strbuf_reset(buf);
-       if (!read_oneliner(buf, rebase_path_strategy(), 0))
-               return;
-       opts->strategy = strbuf_detach(buf, NULL);
-       if (!read_oneliner(buf, rebase_path_strategy_opts(), 0))
-               return;
-
-       parse_strategy_opts(opts, buf->buf);
-}
-
  static int read_populate_opts(struct replay_opts *opts)
  {
         if (is_rebase_i(opts)) {
                 struct strbuf buf = STRBUF_INIT;
                 int ret = 0;

-               if (read_oneliner(&buf, rebase_path_gpg_sign_opt(),
-                                 READ_ONELINER_SKIP_IF_EMPTY)) {
-                       if (!starts_with(buf.buf, "-S"))
-                               strbuf_reset(&buf);
-                       else {
-                               free(opts->gpg_sign);
-                               opts->gpg_sign = xstrdup(buf.buf + 2);
-                       }
-                       strbuf_reset(&buf);
-               }
-
-               if (read_oneliner(&buf, 
rebase_path_allow_rerere_autoupdate(),
-                                 READ_ONELINER_SKIP_IF_EMPTY)) {
-                       if (!strcmp(buf.buf, "--rerere-autoupdate"))
-                               opts->allow_rerere_auto = RERERE_AUTOUPDATE;
-                       else if (!strcmp(buf.buf, "--no-rerere-autoupdate"))
-                               opts->allow_rerere_auto = 
RERERE_NOAUTOUPDATE;
-                       strbuf_reset(&buf);
-               }
-
-               if (file_exists(rebase_path_verbose()))
-                       opts->verbose = 1;
-
-               if (file_exists(rebase_path_quiet()))
-                       opts->quiet = 1;
-
-               if (file_exists(rebase_path_signoff())) {
-                       opts->allow_ff = 0;
-                       opts->signoff = 1;
-               }
-
                 if (file_exists(rebase_path_cdate_is_adate())) {
                         opts->allow_ff = 0;
                         opts->committer_date_is_author_date = 1;
@@ -2959,9 +2916,6 @@ static int read_populate_opts(struct replay_opts 
*opts)
                 if (file_exists(rebase_path_keep_redundant_commits()))
                         opts->keep_redundant_commits = 1;

-               read_strategy_opts(opts, &buf);
-               strbuf_reset(&buf);
-
                 if (read_oneliner(&opts->current_fixups,
                                   rebase_path_current_fixups(),
                                   READ_ONELINER_SKIP_IF_EMPTY)) {

Re: [PATCH 10/17] sequencer.c: fix "opts->strategy" leak in read_strategy_opts()

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 08 2022, Phillip Wood wrote:

> On 04/11/2022 14:50, Phillip Wood wrote:
>> On 03/11/2022 17:06, Ævar Arnfjörð Bjarmason wrote:
>>> When "read_strategy_opts()" is called we may have populated the
>>> "opts->strategy" before, so we'll need to free() it to avoid leaking
>>> memory. 
>> Where is the previous value coming from? I guess it may be the
>> config but otherwise I'm confused.
>
> Having looked a bit more at this I think this is the wrong fix. The
> reason we're overwriting the existing value is that we're reading some 
> of the state files twice. I think the only way to get to this point is
> to go through a code path that calls rebase.c:read_basic_state() which 
> already populates these options via a later call to
> rebase.c:get_replay_opts(). I think the correct fixes looks something 
> like the diff below.
>
> I have also looked at the cherry-pick/revert case and I think that
> we're leaking opts->strategy (and probably some others) when running
>
> 	git cherry-pick --continue
>
> after
>
> 	git -c pull.twohead=recursive cherry-pick -s ort <some commits>

Related: My just-sent [1].

For this one though, I have a v2 re-roll prepared, in which I explained
why the leak is happening (I'll send this soon to the list, but for
now): https://github.com/avar/git/commit/243ab74120b

> I'm not sure what your strategy has been with the fixes in this series
> but we're never going to have 100% coverage of all the option 
> combinations for rebase & cherry-pick so I think it is helpful to
> treat these LSAN reports as a starting point for looking into why the
> leak is occurring and also look for similar leaks.

For this series I'm aiming to solve some common leaks, and increase our
test coverage.

Per [1] the destruction in this area is quite messy, and we should do it
better. But that's a much larger fix. I think in the meantime adding
this free() is the bets way to make incremental progress here, and (an
the commit linked above notes) it's consistent with existing patterns.

1. https://lore.kernel.org/git/221108.864jv9sc9r.gmgdl@evledraar.gmail.com/

[PATCH 11/17] connected.c: free the "struct packed_git"

[Ævar Arnfjörð Bjarmason]

The "new_pack" we allocate in check_connected() wasn't being
free'd. Let's do that before we return from the function. This has
leaked ever since "new_pack" was added to this function in
c6807a40dcd (clone: open a shortcut for connectivity check,
2013-05-26).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 connected.c                         | 6 +++++-
 t/t3050-subprojects-fetch.sh        | 1 +
 t/t4067-diff-partial-clone.sh       | 1 +
 t/t5544-pack-objects-hook.sh        | 2 ++
 t/t5610-clone-detached.sh           | 1 +
 t/t5611-clone-config.sh             | 1 +
 t/t5614-clone-submodules-shallow.sh | 1 +
 t/t5617-clone-submodules-remote.sh  | 1 +
 8 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/connected.c b/connected.c
index 74a20cb32e7..b7770825c7b 100644
--- a/connected.c
+++ b/connected.c
@@ -85,6 +85,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
 promisor_pack_found:
 			;
 		} while ((oid = fn(cb_data)) != NULL);
+		free(new_pack);
 		return 0;
 	}
 
@@ -118,8 +119,10 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
 	else
 		rev_list.no_stderr = opt->quiet;
 
-	if (start_command(&rev_list))
+	if (start_command(&rev_list)) {
+		free(new_pack);
 		return error(_("Could not run 'git rev-list'"));
+	}
 
 	sigchain_push(SIGPIPE, SIG_IGN);
 
@@ -151,5 +154,6 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
 		err = error_errno(_("failed to close rev-list's stdin"));
 
 	sigchain_pop(SIGPIPE);
+	free(new_pack);
 	return finish_command(&rev_list) || err;
 }
diff --git a/t/t3050-subprojects-fetch.sh b/t/t3050-subprojects-fetch.sh
index f1f09abdd9b..38846941655 100755
--- a/t/t3050-subprojects-fetch.sh
+++ b/t/t3050-subprojects-fetch.sh
@@ -2,6 +2,7 @@
 
 test_description='fetching and pushing project with subproject'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '
diff --git a/t/t4067-diff-partial-clone.sh b/t/t4067-diff-partial-clone.sh
index 28f42a4046e..f60f5cbd65f 100755
--- a/t/t4067-diff-partial-clone.sh
+++ b/t/t4067-diff-partial-clone.sh
@@ -2,6 +2,7 @@
 
 test_description='behavior of diff when reading objects in a partial clone'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'git show batches blobs' '
diff --git a/t/t5544-pack-objects-hook.sh b/t/t5544-pack-objects-hook.sh
index 54f54f8d2eb..1a9e14bbccd 100755
--- a/t/t5544-pack-objects-hook.sh
+++ b/t/t5544-pack-objects-hook.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='test custom script in place of pack-objects'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'create some history to fetch' '
diff --git a/t/t5610-clone-detached.sh b/t/t5610-clone-detached.sh
index a7ec21eda5a..022ed3d87c3 100755
--- a/t/t5610-clone-detached.sh
+++ b/t/t5610-clone-detached.sh
@@ -4,6 +4,7 @@ test_description='test cloning a repository with detached HEAD'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 head_is_detached() {
diff --git a/t/t5611-clone-config.sh b/t/t5611-clone-config.sh
index 4b3877216ee..727caff4433 100755
--- a/t/t5611-clone-config.sh
+++ b/t/t5611-clone-config.sh
@@ -4,6 +4,7 @@ test_description='tests for git clone -c key=value'
 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 'clone -c sets config in cloned repo' '
diff --git a/t/t5614-clone-submodules-shallow.sh b/t/t5614-clone-submodules-shallow.sh
index 0c85ef834ab..c2a2bb453ee 100755
--- a/t/t5614-clone-submodules-shallow.sh
+++ b/t/t5614-clone-submodules-shallow.sh
@@ -2,6 +2,7 @@
 
 test_description='Test shallow cloning of repos with submodules'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 pwd=$(pwd)
diff --git a/t/t5617-clone-submodules-remote.sh b/t/t5617-clone-submodules-remote.sh
index 68843382493..5a4d7936a72 100755
--- a/t/t5617-clone-submodules-remote.sh
+++ b/t/t5617-clone-submodules-remote.sh
@@ -5,6 +5,7 @@ test_description='Test cloning repos with submodules using remote-tracking branc
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 pwd=$(pwd)
-- 
2.38.0.1451.g86b35f4140a

[PATCH 12/17] sequencer.c: fix a pick_commits() leak

[Ævar Arnfjörð Bjarmason]

Fix a leak introduced in 1f6965f994f (sequencer: honor
GIT_REFLOG_ACTION, 2020-04-07), we should free() the xstrdup()'d
"prev_reflog_action" string.

We only needed to xstrdup() the previous action if we took this branch
within the "while" statement, and needed to set the GIT_REFLOG_ACTION
for the duration of the do_pick_commit(). Let's just query and restore
it there instead.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 sequencer.c                              | 9 ++++++---
 t/t2012-checkout-last.sh                 | 1 +
 t/t3409-rebase-environ.sh                | 1 +
 t/t3413-rebase-hook.sh                   | 1 +
 t/t3433-rebase-across-mode-change.sh     | 1 +
 t/t7504-commit-msg-hook.sh               | 1 +
 t/t9115-git-svn-dcommit-funky-renames.sh | 1 -
 t/t9146-git-svn-empty-dirs.sh            | 1 -
 t/t9160-git-svn-preserve-empty-dirs.sh   | 1 -
 9 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 07d7062bfb8..14ca0af2ade 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4567,11 +4567,9 @@ static int pick_commits(struct repository *r,
 			struct replay_opts *opts)
 {
 	int res = 0, reschedule = 0;
-	char *prev_reflog_action;
 
 	/* Note that 0 for 3rd parameter of setenv means set only if not set */
 	setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
-	prev_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));
 	if (opts->allow_ff)
 		assert(!(opts->signoff || opts->no_commit ||
 			 opts->record_origin || should_edit(opts) ||
@@ -4618,15 +4616,20 @@ static int pick_commits(struct repository *r,
 			}
 		}
 		if (item->command <= TODO_SQUASH) {
-			if (is_rebase_i(opts))
+			char *prev_reflog_action = NULL;
+
+			if (is_rebase_i(opts)) {
+				prev_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));
 				setenv(GIT_REFLOG_ACTION, reflog_message(opts,
 					command_to_string(item->command), NULL),
 					1);
+			}
 			res = do_pick_commit(r, item, opts,
 					     is_final_fixup(todo_list),
 					     &check_todo);
 			if (is_rebase_i(opts))
 				setenv(GIT_REFLOG_ACTION, prev_reflog_action, 1);
+			free(prev_reflog_action);
 			if (is_rebase_i(opts) && res < 0) {
 				/* Reschedule */
 				advise(_(rescheduled_advice),
diff --git a/t/t2012-checkout-last.sh b/t/t2012-checkout-last.sh
index 1f6c4ed0428..4b6372f4c3e 100755
--- a/t/t2012-checkout-last.sh
+++ b/t/t2012-checkout-last.sh
@@ -5,6 +5,7 @@ test_description='checkout can switch to last branch and merge base'
 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' '
diff --git a/t/t3409-rebase-environ.sh b/t/t3409-rebase-environ.sh
index 83ffb39d9ff..acaf5558dbe 100755
--- a/t/t3409-rebase-environ.sh
+++ b/t/t3409-rebase-environ.sh
@@ -2,6 +2,7 @@
 
 test_description='git rebase interactive environment'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t3413-rebase-hook.sh b/t/t3413-rebase-hook.sh
index 9fab0d779bb..e8456831e8b 100755
--- a/t/t3413-rebase-hook.sh
+++ b/t/t3413-rebase-hook.sh
@@ -5,6 +5,7 @@ test_description='git rebase with its hook(s)'
 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 '
diff --git a/t/t3433-rebase-across-mode-change.sh b/t/t3433-rebase-across-mode-change.sh
index 05df964670f..c8172b08522 100755
--- a/t/t3433-rebase-across-mode-change.sh
+++ b/t/t3433-rebase-across-mode-change.sh
@@ -2,6 +2,7 @@
 
 test_description='git rebase across mode change'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh
index a39de8c1126..07ca46fb0d5 100755
--- a/t/t7504-commit-msg-hook.sh
+++ b/t/t7504-commit-msg-hook.sh
@@ -5,6 +5,7 @@ test_description='commit-msg hook'
 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 'with no hook' '
diff --git a/t/t9115-git-svn-dcommit-funky-renames.sh b/t/t9115-git-svn-dcommit-funky-renames.sh
index 419f055721d..743fbe1fe46 100755
--- a/t/t9115-git-svn-dcommit-funky-renames.sh
+++ b/t/t9115-git-svn-dcommit-funky-renames.sh
@@ -5,7 +5,6 @@
 
 test_description='git svn dcommit can commit renames of files with ugly names'
 
-TEST_FAILS_SANITIZE_LEAK=true
 . ./lib-git-svn.sh
 
 test_expect_success 'load repository with strange names' '
diff --git a/t/t9146-git-svn-empty-dirs.sh b/t/t9146-git-svn-empty-dirs.sh
index 79c26ed69c1..09606f1b3cf 100755
--- a/t/t9146-git-svn-empty-dirs.sh
+++ b/t/t9146-git-svn-empty-dirs.sh
@@ -4,7 +4,6 @@
 
 test_description='git svn creates empty directories'
 
-TEST_FAILS_SANITIZE_LEAK=true
 . ./lib-git-svn.sh
 
 test_expect_success 'initialize repo' '
diff --git a/t/t9160-git-svn-preserve-empty-dirs.sh b/t/t9160-git-svn-preserve-empty-dirs.sh
index 9cf7a1427ab..36c6b1a12ff 100755
--- a/t/t9160-git-svn-preserve-empty-dirs.sh
+++ b/t/t9160-git-svn-preserve-empty-dirs.sh
@@ -9,7 +9,6 @@ This test uses git to clone a Subversion repository that contains empty
 directories, and checks that corresponding directories are created in the
 local Git repository with placeholder files.'
 
-TEST_FAILS_SANITIZE_LEAK=true
 . ./lib-git-svn.sh
 
 GIT_REPO=git-svn-repo
-- 
2.38.0.1451.g86b35f4140a

[PATCH 13/17] rebase: don't leak on "--abort"

[Ævar Arnfjörð Bjarmason]

Fix a leak in the recent 6159e7add49 (rebase --abort: improve reflog
message, 2022-10-12). Before that commit we'd strbuf_release() the
reflog message we were formatting, but when that code was refactored
to use "ropts.head_msg" the strbuf_release() was omitted.

Ideally the three users of "ropts" in cmd_rebase() should use
different "ropts" variables, in practice they're completely separate,
as this and the other user in the "switch" statement will "goto
cleanup", which won't touch "ropts".

The third caller after the "switch" is then unreachable if we take
these two branches, so all of them are getting a "{ 0 }" init'd
"ropts".

So it's OK that we're leaving a stale pointer in "ropts.head_msg",
cleaning it up was our responsibility, and it won't be used again.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/rebase.c          | 1 +
 t/t7517-per-repo-email.sh | 1 +
 2 files changed, 2 insertions(+)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 64aed11c85d..8a8b5e34786 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1320,6 +1320,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		if (reset_head(the_repository, &ropts) < 0)
 			die(_("could not move back to %s"),
 			    oid_to_hex(&options.orig_head->object.oid));
+		strbuf_release(&head_msg);
 		remove_branch_state(the_repository, 0);
 		ret = finish_rebase(&options);
 		goto cleanup;
diff --git a/t/t7517-per-repo-email.sh b/t/t7517-per-repo-email.sh
index 163ae804685..efc6496e2b2 100755
--- a/t/t7517-per-repo-email.sh
+++ b/t/t7517-per-repo-email.sh
@@ -9,6 +9,7 @@ test_description='per-repo forced setting of email address'
 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 a likely user.useConfigOnly use case' '
-- 
2.38.0.1451.g86b35f4140a

Re: [PATCH 13/17] rebase: don't leak on "--abort"

[Phillip Wood]

Hi Ævar

On 03/11/2022 17:06, Ævar Arnfjörð Bjarmason wrote:
> Fix a leak in the recent 6159e7add49 (rebase --abort: improve reflog
> message, 2022-10-12). Before that commit we'd strbuf_release() the
> reflog message we were formatting, but when that code was refactored
> to use "ropts.head_msg" the strbuf_release() was omitted.
> 
> Ideally the three users of "ropts" in cmd_rebase() should use
> different "ropts" variables, in practice they're completely separate,
> as this and the other user in the "switch" statement will "goto
> cleanup", which won't touch "ropts".
> 
> The third caller after the "switch" is then unreachable if we take
> these two branches, so all of them are getting a "{ 0 }" init'd
> "ropts".
> 
> So it's OK that we're leaving a stale pointer in "ropts.head_msg",
> cleaning it up was our responsibility, and it won't be used again.

This looks fine. One could argue that the leak is not a "real" leak as 
we're about to exit but the omission of strbuf_release() was 
unintentional and fix is nice and simple.

Thanks

Phillip

> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>   builtin/rebase.c          | 1 +
>   t/t7517-per-repo-email.sh | 1 +
>   2 files changed, 2 insertions(+)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 64aed11c85d..8a8b5e34786 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1320,6 +1320,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   		if (reset_head(the_repository, &ropts) < 0)
>   			die(_("could not move back to %s"),
>   			    oid_to_hex(&options.orig_head->object.oid));
> +		strbuf_release(&head_msg);
>   		remove_branch_state(the_repository, 0);
>   		ret = finish_rebase(&options);
>   		goto cleanup;
> diff --git a/t/t7517-per-repo-email.sh b/t/t7517-per-repo-email.sh
> index 163ae804685..efc6496e2b2 100755
> --- a/t/t7517-per-repo-email.sh
> +++ b/t/t7517-per-repo-email.sh
> @@ -9,6 +9,7 @@ test_description='per-repo forced setting of email address'
>   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 a likely user.useConfigOnly use case' '

Re: [PATCH 13/17] rebase: don't leak on "--abort"

[Ævar Arnfjörð Bjarmason]

On Fri, Nov 04 2022, Phillip Wood wrote:

> On 03/11/2022 17:06, Ævar Arnfjörð Bjarmason wrote:
>> Fix a leak in the recent 6159e7add49 (rebase --abort: improve reflog
>> message, 2022-10-12). Before that commit we'd strbuf_release() the
>> reflog message we were formatting, but when that code was refactored
>> to use "ropts.head_msg" the strbuf_release() was omitted.
>> Ideally the three users of "ropts" in cmd_rebase() should use
>> different "ropts" variables, in practice they're completely separate,
>> as this and the other user in the "switch" statement will "goto
>> cleanup", which won't touch "ropts".
>> The third caller after the "switch" is then unreachable if we take
>> these two branches, so all of them are getting a "{ 0 }" init'd
>> "ropts".
>> So it's OK that we're leaving a stale pointer in "ropts.head_msg",
>> cleaning it up was our responsibility, and it won't be used again.
>> [...]
>> --- a/builtin/rebase.c
>> +++ b/builtin/rebase.c
>> @@ -1320,6 +1320,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>>   		if (reset_head(the_repository, &ropts) < 0)
>>   			die(_("could not move back to %s"),
>>   			    oid_to_hex(&options.orig_head->object.oid));
>> +		strbuf_release(&head_msg);
>>   		remove_branch_state(the_repository, 0);
>>   		ret = finish_rebase(&options);
>>   		goto cleanup;
>
> This looks fine. One could argue that the leak is not a "real" leak as
> we're about to exit but the omission of strbuf_release() was 
> unintentional and fix is nice and simple.

What I've been targeting in this and past leak fix topics is roughly
what valgrind calls[1] "definitely lost" and "indirectly lost", and
which -fsanitize=leak calls "that thing I die on whan I run into it" :)

So, you and I know we're "about to exit" as we're in cmd_rebase() and
about to return to common-main.c etc, but it's harder to convince a
compiler of that.

Maybe I'm just touchy, sorry. It just feels like every time I submit a
leak fix topic there's some rehash of the "why do this at all?" or "why
this one?"  discussion.

To summarize (maybe too much for just this reply, but I hope to link
back to this in the future):

 * Historically we've said "that's just a built-in, let the OS take care
   of it!"

 * I agree with that in principle. It's pretty pointless to free()
   things in cmd_*() in the abstract. We're "about to exit", even if
   valgrind etc. call it "definitely lost"[1].

 * But, it would be nice to have git's APIs not leak, so we could call
   them in loops, replace some of our own sub-process invocations
   etc[2].

 * If we had 100% test coverage for those libraries we could just test
   that, and get those leak-free, and ignore the built-ins.

 * We don't have that, and probably never will. E.g. "git log" and
   friends are *the* things that stress revision.[ch]. So to assert that
   the API is leak-free we really need to assert that its users
   are. Ditto for pretty much any other API we carry.

 * Still, we could just say "if something's alloc'd in cmd_*() don't
   care about it". Both LSAN and valgrind support "suppressions" to
   ignore certain functions, patterns etc.

 * But that doesn't really work either, as e.g. "struct rev_info" would
   seem to come from such a cmd_*() as far as ownership goes...

 * ..."but you could have an even more clever suppressions mechanism,
   and only ignore those things that are malloc'd by cmd_*(). But not
   e.g. a malloc in revision.c in a struct member in a struct that's on
   the stack in a cmd_*()", one might say.

   Yeah, you could do that, e.g. in this case we'll call strbuf_addf()
   which makes a beeline to malloc(). Sufficiently clever
   post-processing of leak stacktraces could probably make the
   distinction.

   But then you run into the problem of e.g. how the the log family and
   others use "mailmap". I.e. in that case (and many others) the cmd_*()
   will malloc(), but "hand it off" to the API, whose job we know it is
   to free() it.

   But we only know because of knowledge about the API, an automated
   system would care, still, it's not unworkable. We could go through
   those, list the exceptions etc. etc.

That's a lot of context, but I think brings us up-to-date on this
one-line change.

So, could we do things differently here? Definitely. And if the codebase
was in a state where e.g. builtin/*.c had ~1500 UNLEAK() already I'd
probably try to extend that, rather than fighting that uphill battle.

But that's not the case, we have around ~30 of them, fewer after this
topic. By far the majority of builtin/* is caring about directly
recahable leaks already.

E.g. just above this context in another "case" arm we're doing the exact
"variable on stack, use it, then release()", just with a "struct
string_list".

So I think this is the right direction to go in, both in this patch & in
general.

1. https://valgrind.org/docs/manual/mc-manual.html#mc-manual.leaks
2. Even the assumption that cmd_*() is a one-off is shaky, e.g. for
   cases where we shell out to "git commit" we're close now to being
   able to just call cmd_commit(), but let's leave that aside for now).

   So, we're pretty close to cmd_*() being a funny API that happens to
   take strvec arguments, which is going to be the shortest path to
   eliminating sub-process invocation in many cases. I.e. as opposed to
   refactoring the cmd_*() into a "real" library, that takes a struct
   with options, doesn't call parse_options() etc.

[PATCH 14/17] sequencer.c: fix sequencer_continue() leak

[Ævar Arnfjörð Bjarmason]

Fix a leak in the recent da1d63363f1 (rebase --merge: fix reflog when
continuing, 2022-10-12), per [1] the author was under the impression
that by calling "setenv()" the C library took possession of the
string, but we need to free() it.

As [1] also notes there's upcoming changes to do some larger rewrite
of these codepaths in sequencer.c, but let's first do this much
smaller and isolated leak fix.

1. https://lore.kernel.org/git/86699708-d631-fb49-482c-af27204a3570@dunelm.org.uk/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 sequencer.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sequencer.c b/sequencer.c
index 14ca0af2ade..3095d0d2b3b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5069,10 +5069,12 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)
 		previous_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));
 		setenv(GIT_REFLOG_ACTION, reflog_message(opts, "continue", NULL), 1);
 		if (commit_staged_changes(r, opts, &todo_list)) {
+			free(previous_reflog_action);
 			res = -1;
 			goto release_todo_list;
 		}
 		setenv(GIT_REFLOG_ACTION, previous_reflog_action, 1);
+		free(previous_reflog_action);
 	} else if (!file_exists(get_todo_path(opts)))
 		return continue_single_pick(r, opts);
 	else if ((res = read_populate_todo(r, &todo_list, opts)))
-- 
2.38.0.1451.g86b35f4140a

[PATCH 15/17] cherry-pick: free "struct replay_opts" members

[Ævar Arnfjörð Bjarmason]

Call the release_revisions() function added in
1878b5edc03 (revision.[ch]: provide and start using a
release_revisions(), 2022-04-13) in cmd_cherry_pick(), as well as
freeing the xmalloc()'d "revs" member itself.

This is the same change as the one made for cmd_revert() a few lines
above it in fd74ac95ac3 (revert: free "struct replay_opts" members,
2022-07-01).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/revert.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/builtin/revert.c b/builtin/revert.c
index ee32c714a76..0f81c8a795a 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -261,6 +261,9 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 	opts.action = REPLAY_PICK;
 	sequencer_init_config(&opts);
 	res = run_sequencer(argc, argv, &opts);
+	if (opts.revs)
+		release_revisions(opts.revs);
+	free(opts.revs);
 	if (res < 0)
 		die(_("cherry-pick failed"));
 	return res;
-- 
2.38.0.1451.g86b35f4140a

[PATCH 16/17] revert: fix parse_options_concat() leak

[Ævar Arnfjörð Bjarmason]

Free memory from parse_options_concat(), which comes from code
originally added (then extended) in [1].

At this point we could get several more tests leak-free by free()-ing
the xstrdup() just above the line being changed, but that one's
trickier than it seems. The sequencer_remove_state() function
supposedly owns it, but sometimes we don't call it. I have a fix for
it, but it's non-trivial, so let's fix the easy one first.

1. c62f6ec341b (revert: add --ff option to allow fast forward when
   cherry-picking, 2010-03-06)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/revert.c            | 1 +
 t/t3429-rebase-edit-todo.sh | 1 +
 2 files changed, 2 insertions(+)

diff --git a/builtin/revert.c b/builtin/revert.c
index 0f81c8a795a..8bc87e4c770 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -221,6 +221,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
 	opts->strategy = xstrdup_or_null(opts->strategy);
 	if (!opts->strategy && getenv("GIT_TEST_MERGE_ALGORITHM"))
 		opts->strategy = xstrdup(getenv("GIT_TEST_MERGE_ALGORITHM"));
+	free(options);
 
 	if (cmd == 'q') {
 		int ret = sequencer_remove_state(opts);
diff --git a/t/t3429-rebase-edit-todo.sh b/t/t3429-rebase-edit-todo.sh
index abd66f36021..8e0d03969a2 100755
--- a/t/t3429-rebase-edit-todo.sh
+++ b/t/t3429-rebase-edit-todo.sh
@@ -2,6 +2,7 @@
 
 test_description='rebase should reread the todo file if an exec modifies it'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-rebase.sh
 
-- 
2.38.0.1451.g86b35f4140a

[PATCH 17/17] built-ins: use free() not UNLEAK() if trivial, rm dead code

[Ævar Arnfjörð Bjarmason]

For a lot of uses of UNLEAK() it would be quite tricky to release the
memory involved, or we're missing the relevant *_(release|clear)()
functions. But in these cases we have them already, and can just
invoke them on the variable(s) involved, instead of UNLEAK().

For "builtin/worktree.c" the UNLEAK() was also added in [1], but the
struct member it's unleaking was removed in [2]. The only non-"int"
member of that structure is "const char *keep_locked", which comes to
us via "argv" or a string literal[3].

We have good visibility via the compiler and
tooling (e.g. SANITIZE=address) on bad free()-ing, but none on
UNLEAK() we don't need anymore. So let's prefer releasing the memory
when it's easy.

For "bugreport", "worktree" and "config" we need to start using a "ret
= ..." return pattern. For "builtin/bugreport.c" these UNLEAK() were
added in [4], and for "builtin/config.c" in [1].

For "config" the code seen here was the only user of the "value"
variable. For "ACTION_{RENAME,REMOVE}_SECTION" we need to be sure to
return the right exit code in the cases where we were relying on
falling through to the top-level.

I think there's still a use-case for UNLEAK(), but hat it's changed
since then. Using it so that "we can see the real leaks" is
counter-productive in these cases.

It's more useful to have UNLEAK() be a marker of the remaining odd
cases where it's hard to free() the memory for whatever reason. With
this change less than 20 of them remain in-tree.

1. 0e5bba53af7 (add UNLEAK annotation for reducing leak false
   positives, 2017-09-08)
2. d861d34a6ed (worktree: remove extra members from struct add_opts,
   2018-04-24)
3. 0db4961c49b (worktree: teach `add` to accept --reason <string> with
  --lock, 2021-07-15)
4. 0e5bba53af7 and 00d8c311050 (commit: fix "author_ident" leak,
   2022-05-12).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/add.c       |  2 +-
 builtin/bugreport.c |  9 ++++++---
 builtin/commit.c    |  6 +++---
 builtin/config.c    | 42 ++++++++++++++++++++----------------------
 builtin/diff.c      |  2 +-
 builtin/worktree.c  |  7 ++++---
 6 files changed, 35 insertions(+), 33 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index f84372964c8..c68ebafed5e 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -695,6 +695,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		die(_("Unable to write new index file"));
 
 	dir_clear(&dir);
-	UNLEAK(pathspec);
+	clear_pathspec(&pathspec);
 	return exit_status;
 }
diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index 96052541cbf..5bc254be80f 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -106,6 +106,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
 	const char *user_relative_path = NULL;
 	char *prefixed_filename;
 	size_t output_path_len;
+	int ret;
 
 	const struct option bugreport_options[] = {
 		OPT_CALLBACK_F(0, "diagnose", &diagnose, N_("mode"),
@@ -182,7 +183,9 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
 		user_relative_path);
 
 	free(prefixed_filename);
-	UNLEAK(buffer);
-	UNLEAK(report_path);
-	return !!launch_editor(report_path.buf, NULL, NULL);
+	strbuf_release(&buffer);
+
+	ret = !!launch_editor(report_path.buf, NULL, NULL);
+	strbuf_release(&report_path);
+	return ret;
 }
diff --git a/builtin/commit.c b/builtin/commit.c
index c291199b704..f88a29167f4 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1874,8 +1874,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	apply_autostash(git_path_merge_autostash(the_repository));
 
 cleanup:
-	UNLEAK(author_ident);
-	UNLEAK(err);
-	UNLEAK(sb);
+	strbuf_release(&author_ident);
+	strbuf_release(&err);
+	strbuf_release(&sb);
 	return ret;
 }
diff --git a/builtin/config.c b/builtin/config.c
index 753e5fac297..060cf9f3e05 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -639,8 +639,9 @@ static char *default_user_config(void)
 int cmd_config(int argc, const char **argv, const char *prefix)
 {
 	int nongit = !startup_info->have_repository;
-	char *value;
+	char *value = NULL;
 	int flags = 0;
+	int ret = 0;
 
 	given_config_source.file = xstrdup_or_null(getenv(CONFIG_ENVIRONMENT));
 
@@ -856,44 +857,38 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		free(config_file);
 	}
 	else if (actions == ACTION_SET) {
-		int ret;
 		check_write();
 		check_argc(argc, 2, 2);
 		value = normalize_value(argv[0], argv[1]);
-		UNLEAK(value);
 		ret = git_config_set_in_file_gently(given_config_source.file, argv[0], value);
 		if (ret == CONFIG_NOTHING_SET)
 			error(_("cannot overwrite multiple values with a single value\n"
 			"       Use a regexp, --add or --replace-all to change %s."), argv[0]);
-		return ret;
 	}
 	else if (actions == ACTION_SET_ALL) {
 		check_write();
 		check_argc(argc, 2, 3);
 		value = normalize_value(argv[0], argv[1]);
-		UNLEAK(value);
-		return git_config_set_multivar_in_file_gently(given_config_source.file,
-							      argv[0], value, argv[2],
-							      flags);
+		ret = git_config_set_multivar_in_file_gently(given_config_source.file,
+							     argv[0], value, argv[2],
+							     flags);
 	}
 	else if (actions == ACTION_ADD) {
 		check_write();
 		check_argc(argc, 2, 2);
 		value = normalize_value(argv[0], argv[1]);
-		UNLEAK(value);
-		return git_config_set_multivar_in_file_gently(given_config_source.file,
-							      argv[0], value,
-							      CONFIG_REGEX_NONE,
-							      flags);
+		ret = git_config_set_multivar_in_file_gently(given_config_source.file,
+							     argv[0], value,
+							     CONFIG_REGEX_NONE,
+							     flags);
 	}
 	else if (actions == ACTION_REPLACE_ALL) {
 		check_write();
 		check_argc(argc, 2, 3);
 		value = normalize_value(argv[0], argv[1]);
-		UNLEAK(value);
-		return git_config_set_multivar_in_file_gently(given_config_source.file,
-							      argv[0], value, argv[2],
-							      flags | CONFIG_FLAGS_MULTI_REPLACE);
+		ret = git_config_set_multivar_in_file_gently(given_config_source.file,
+							     argv[0], value, argv[2],
+							     flags | CONFIG_FLAGS_MULTI_REPLACE);
 	}
 	else if (actions == ACTION_GET) {
 		check_argc(argc, 1, 2);
@@ -934,26 +929,28 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 							      flags | CONFIG_FLAGS_MULTI_REPLACE);
 	}
 	else if (actions == ACTION_RENAME_SECTION) {
-		int ret;
 		check_write();
 		check_argc(argc, 2, 2);
 		ret = git_config_rename_section_in_file(given_config_source.file,
 							argv[0], argv[1]);
 		if (ret < 0)
 			return ret;
-		if (ret == 0)
+		else if (!ret)
 			die(_("no such section: %s"), argv[0]);
+		else
+			ret = 0;
 	}
 	else if (actions == ACTION_REMOVE_SECTION) {
-		int ret;
 		check_write();
 		check_argc(argc, 1, 1);
 		ret = git_config_rename_section_in_file(given_config_source.file,
 							argv[0], NULL);
 		if (ret < 0)
 			return ret;
-		if (ret == 0)
+		else if (!ret)
 			die(_("no such section: %s"), argv[0]);
+		else
+			ret = 0;
 	}
 	else if (actions == ACTION_GET_COLOR) {
 		check_argc(argc, 1, 2);
@@ -966,5 +963,6 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		return get_colorbool(argv[0], argc == 2);
 	}
 
-	return 0;
+	free(value);
+	return ret;
 }
diff --git a/builtin/diff.c b/builtin/diff.c
index 854d2c5a5c4..cb63f157dd1 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -609,7 +609,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	if (1 < rev.diffopt.skip_stat_unmatch)
 		refresh_index_quietly();
 	release_revisions(&rev);
-	UNLEAK(ent);
+	object_array_clear(&ent);
 	UNLEAK(blob);
 	return result;
 }
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 4a24d53be15..591d659faea 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -629,6 +629,7 @@ static int add(int ac, const char **av, const char *prefix)
 			 N_("try to match the new branch name with a remote-tracking branch")),
 		OPT_END()
 	};
+	int ret;
 
 	memset(&opts, 0, sizeof(opts));
 	opts.checkout = 1;
@@ -705,9 +706,9 @@ static int add(int ac, const char **av, const char *prefix)
 		die(_("--[no-]track can only be used if a new branch is created"));
 	}
 
-	UNLEAK(path);
-	UNLEAK(opts);
-	return add_worktree(path, branch, &opts);
+	ret = add_worktree(path, branch, &opts);
+	free(path);
+	return ret;
 }
 
 static void show_worktree_porcelain(struct worktree *wt, int line_terminator)
-- 
2.38.0.1451.g86b35f4140a

Re: [PATCH 00/17] leak fixes: use existing constructors & other trivia

[Phillip Wood]

On 03/11/2022 17:05, Ævar Arnfjörð Bjarmason wrote:
> With the very minor exceptions of:
> 
> * 03-04/17 (which need trivial oilerplate)
> * 05/17 (need to add trivial control flow to a free_*() function)
> * 12/17 (narrowing scope of allocation)

I've only looked at the rebase related patches. I'd really appreciate it 
if you could drop patches 12 & 14 as they conflict with [1] that fixes 
these issues by removing the setenv() calls.

Thanks

Phillip

[1] 
https://lore.kernel.org/git/pull.1405.git.1667575142.gitgitgadget@gmail.com

> * 17/17: Add "goto ret" pattern, combine two "ret" variables
> 
> These are all "one-line" leak fixes where we merely need to make use
> of an existing release function. The "one-line" only having the slight
> disclaimer of needing to e.g. add braces to an "if" in one case, etc.
> 
> Each commit in this series is tested with:
> 
> 	GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_SANITIZE_LEAK_LOG=true \
> 	make SANITIZE=leak test
> 
> I.e. mark tests as leak-free as we fix the leaks.
> 
> In 17/17 I replace uses of UNLEAK() where we can just as easily free()
> instead, i.e. most of it's built-ins doing UNLEAK(x) instead of
> strbuf_release(&x) etc.
> 
> As 17/17 notes I still think these's a place for unleak (some of the
> remaining ones are quite tricky), but that we gain more from leaving
> it for those tricky cases. Before this series we have 28 uses of
> UNLEAK(), after it's 15.
> 
> Ævar Arnfjörð Bjarmason (17):
>    tests: mark tests as passing with SANITIZE=leak
>    {reset,merge}: call discard_index() before returning
>    commit: discard partial cache before (re-)reading it
>    read-cache.c: clear and free "sparse_checkout_patterns"
>    dir.c: free "ident" and "exclude_per_dir" in "struct untracked_cache"
>    built-ins & libs & helpers: add/move destructors, fix leaks
>    unpack-file: fix ancient leak in create_temp_file()
>    revision API: call graph_clear() in release_revisions()
>    ls-files: fix a --with-tree memory leak
>    sequencer.c: fix "opts->strategy" leak in read_strategy_opts()
>    connected.c: free the "struct packed_git"
>    sequencer.c: fix a pick_commits() leak
>    rebase: don't leak on "--abort"
>    sequencer.c: fix sequencer_continue() leak
>    cherry-pick: free "struct replay_opts" members
>    revert: fix parse_options_concat() leak
>    built-ins: use free() not UNLEAK() if trivial, rm dead code
> 
>   builtin/add.c                            |  2 +-
>   builtin/bugreport.c                      |  9 +++--
>   builtin/checkout.c                       |  2 ++
>   builtin/commit.c                         | 13 +++++---
>   builtin/config.c                         | 42 +++++++++++-------------
>   builtin/diff.c                           |  2 +-
>   builtin/ls-files.c                       |  1 +
>   builtin/merge.c                          |  1 +
>   builtin/rebase.c                         |  4 +++
>   builtin/repack.c                         |  2 +-
>   builtin/reset.c                          |  2 ++
>   builtin/rev-parse.c                      |  1 +
>   builtin/revert.c                         |  4 +++
>   builtin/stash.c                          |  2 ++
>   builtin/unpack-file.c                    |  1 +
>   builtin/worktree.c                       |  7 ++--
>   connected.c                              |  6 +++-
>   dir.c                                    | 10 ++++--
>   dir.h                                    |  1 +
>   read-cache.c                             |  5 +++
>   ref-filter.c                             |  1 +
>   revision.c                               |  1 +
>   sequencer.c                              | 12 +++++--
>   t/helper/test-fake-ssh.c                 |  1 +
>   t/t0068-for-each-repo.sh                 |  1 +
>   t/t0070-fundamental.sh                   |  1 +
>   t/t1011-read-tree-sparse-checkout.sh     |  1 +
>   t/t1022-read-tree-partial-clone.sh       |  2 +-
>   t/t1404-update-ref-errors.sh             |  2 ++
>   t/t1409-avoid-packing-refs.sh            |  1 +
>   t/t1413-reflog-detach.sh                 |  1 +
>   t/t1501-work-tree.sh                     |  2 ++
>   t/t2012-checkout-last.sh                 |  1 +
>   t/t2018-checkout-branch.sh               |  1 +
>   t/t2025-checkout-no-overlay.sh           |  1 +
>   t/t3009-ls-files-others-nonsubmodule.sh  |  1 +
>   t/t3010-ls-files-killed-modified.sh      |  2 ++
>   t/t3050-subprojects-fetch.sh             |  1 +
>   t/t3060-ls-files-with-tree.sh            |  2 ++
>   t/t3409-rebase-environ.sh                |  1 +
>   t/t3413-rebase-hook.sh                   |  1 +
>   t/t3428-rebase-signoff.sh                |  1 +
>   t/t3429-rebase-edit-todo.sh              |  1 +
>   t/t3433-rebase-across-mode-change.sh     |  1 +
>   t/t4015-diff-whitespace.sh               |  4 +--
>   t/t4045-diff-relative.sh                 |  2 ++
>   t/t4052-stat-output.sh                   |  1 +
>   t/t4053-diff-no-index.sh                 |  1 +
>   t/t4067-diff-partial-clone.sh            |  1 +
>   t/t4111-apply-subdir.sh                  |  1 +
>   t/t4135-apply-weird-filenames.sh         |  1 +
>   t/t4213-log-tabexpand.sh                 |  1 +
>   t/t5544-pack-objects-hook.sh             |  2 ++
>   t/t5554-noop-fetch-negotiator.sh         |  2 ++
>   t/t5610-clone-detached.sh                |  1 +
>   t/t5611-clone-config.sh                  |  1 +
>   t/t5614-clone-submodules-shallow.sh      |  1 +
>   t/t5617-clone-submodules-remote.sh       |  1 +
>   t/t5618-alternate-refs.sh                |  2 ++
>   t/t6060-merge-index.sh                   |  2 ++
>   t/t6301-for-each-ref-errors.sh           |  1 +
>   t/t6401-merge-criss-cross.sh             |  2 ++
>   t/t6406-merge-attr.sh                    |  1 +
>   t/t6407-merge-binary.sh                  |  1 +
>   t/t6415-merge-dir-to-symlink.sh          |  1 +
>   t/t6435-merge-sparse.sh                  |  1 +
>   t/t7103-reset-bare.sh                    |  2 +-
>   t/t7504-commit-msg-hook.sh               |  1 +
>   t/t7517-per-repo-email.sh                |  1 +
>   t/t7520-ignored-hook-warning.sh          |  1 +
>   t/t7605-merge-resolve.sh                 |  1 +
>   t/t7614-merge-signoff.sh                 |  1 +
>   t/t9003-help-autocorrect.sh              |  2 ++
>   t/t9115-git-svn-dcommit-funky-renames.sh |  1 -
>   t/t9146-git-svn-empty-dirs.sh            |  1 -
>   t/t9148-git-svn-propset.sh               |  1 -
>   t/t9160-git-svn-preserve-empty-dirs.sh   |  1 -
>   77 files changed, 150 insertions(+), 51 deletions(-)
> 

Re: [PATCH 00/17] leak fixes: use existing constructors & other trivia

[Ævar Arnfjörð Bjarmason]

On Fri, Nov 04 2022, Phillip Wood wrote:

> On 03/11/2022 17:05, Ævar Arnfjörð Bjarmason wrote:
>> With the very minor exceptions of:
>> * 03-04/17 (which need trivial oilerplate)
>> * 05/17 (need to add trivial control flow to a free_*() function)
>> * 12/17 (narrowing scope of allocation)
>
> I've only looked at the rebase related patches. I'd really appreciate
> it if you could drop patches 12 & 14 as they conflict with [1] that
> fixes these issues by removing the setenv() calls.

Sure, sounds good to me.

I see Taylor queued it up like that, and the combination of the two
passes with the "true" and "check" mode leak checking modes.

For context: I saw that you planned to fix those in some follow-up
topic, but had this mostly ready before the new leak in the recent topic
showed up, and that leak had big knock-on effects in what test files I
could mark as passing.

So I figured I'd include some more more narrow fixes, but I'm even
happier to see the root cause addressed.

Re: [PATCH 00/17] leak fixes: use existing constructors & other trivia

[Phillip Wood]

On 05/11/2022 12:46, Ævar Arnfjörð Bjarmason wrote:
> 
> On Fri, Nov 04 2022, Phillip Wood wrote:
> 
>> On 03/11/2022 17:05, Ævar Arnfjörð Bjarmason wrote:
>>> With the very minor exceptions of:
>>> * 03-04/17 (which need trivial oilerplate)
>>> * 05/17 (need to add trivial control flow to a free_*() function)
>>> * 12/17 (narrowing scope of allocation)
>>
>> I've only looked at the rebase related patches. I'd really appreciate
>> it if you could drop patches 12 & 14 as they conflict with [1] that
>> fixes these issues by removing the setenv() calls.
> 
> Sure, sounds good to me.

Thanks

> I see Taylor queued it up like that, and the combination of the two
> passes with the "true" and "check" mode leak checking modes.

That's great

Best Wishes

Phillip


> For context: I saw that you planned to fix those in some follow-up
> topic, but had this mostly ready before the new leak in the recent topic
> showed up, and that leak had big knock-on effects in what test files I
> could mark as passing.
> 
> So I figured I'd include some more more narrow fixes, but I'm even
> happier to see the root cause addressed.

[PATCH v2 00/15] leak fixes: use existing constructors & other trivia

[Ævar Arnfjörð Bjarmason]

This is a bag of simple leak fixes, mostly one-line additions where we
use existing destructors. See [1] for v1.

Changes since v1:

 * Rebased on pw/rebase-no-reflog-action, per [2].
 * It'll merge just fine without pw/rebase-no-reflog-action, but some
   of the tests marked as leak-free will still leak then. So, in
   practice they can be staged independently, as long as
   pw/rebase-no-reflog-action is merged down first.
 * The 10/15 here has an updated explanation about why we were leaking
   that value. see also[3].

As noted in my [4] some of the rebase/sequencer leaks addressed here
should have a more thorough fix to address the root cause of some of
them, which is also something pw/rebase-no-reflog-action is running
into (and why it's introducing a new leak, while fixing another one).

But per [4] I think the right thing to do is to merge both that topic
& this down first, and then we can follow-up with those fixes (which I
have staged locally already). Getting the extra leak coverage this
series & pw/rebase-no-reflog-action bring us will help in the
meantime.

1. https://lore.kernel.org/git/cover-00.17-00000000000-20221103T164632Z-avarab@gmail.com/
2. https://lore.kernel.org/git/8eec228d-d392-523d-2415-149b946f642e@dunelm.org.uk/
3. https://lore.kernel.org/git/221108.86zgd1qxac.gmgdl@evledraar.gmail.com/
4. https://lore.kernel.org/git/221108.864jv9sc9r.gmgdl@evledraar.gmail.com/

Ævar Arnfjörð Bjarmason (15):
  tests: mark tests as passing with SANITIZE=leak
  {reset,merge}: call discard_index() before returning
  commit: discard partial cache before (re-)reading it
  read-cache.c: clear and free "sparse_checkout_patterns"
  dir.c: free "ident" and "exclude_per_dir" in "struct untracked_cache"
  built-ins & libs & helpers: add/move destructors, fix leaks
  unpack-file: fix ancient leak in create_temp_file()
  revision API: call graph_clear() in release_revisions()
  ls-files: fix a --with-tree memory leak
  sequencer.c: fix "opts->strategy" leak in read_strategy_opts()
  connected.c: free the "struct packed_git"
  rebase: don't leak on "--abort"
  cherry-pick: free "struct replay_opts" members
  revert: fix parse_options_concat() leak
  built-ins: use free() not UNLEAK() if trivial, rm dead code

 builtin/add.c                            |  2 +-
 builtin/bugreport.c                      |  9 +++--
 builtin/checkout.c                       |  2 ++
 builtin/commit.c                         | 13 +++++---
 builtin/config.c                         | 42 +++++++++++-------------
 builtin/diff.c                           |  2 +-
 builtin/ls-files.c                       |  1 +
 builtin/merge.c                          |  1 +
 builtin/rebase.c                         |  4 +++
 builtin/repack.c                         |  2 +-
 builtin/reset.c                          |  2 ++
 builtin/rev-parse.c                      |  1 +
 builtin/revert.c                         |  4 +++
 builtin/stash.c                          |  2 ++
 builtin/unpack-file.c                    |  1 +
 builtin/worktree.c                       |  7 ++--
 connected.c                              |  6 +++-
 dir.c                                    | 10 ++++--
 dir.h                                    |  1 +
 read-cache.c                             |  5 +++
 ref-filter.c                             |  1 +
 revision.c                               |  1 +
 sequencer.c                              |  1 +
 t/helper/test-fake-ssh.c                 |  1 +
 t/t0068-for-each-repo.sh                 |  1 +
 t/t0070-fundamental.sh                   |  1 +
 t/t1011-read-tree-sparse-checkout.sh     |  1 +
 t/t1022-read-tree-partial-clone.sh       |  2 +-
 t/t1404-update-ref-errors.sh             |  2 ++
 t/t1409-avoid-packing-refs.sh            |  1 +
 t/t1413-reflog-detach.sh                 |  1 +
 t/t1501-work-tree.sh                     |  2 ++
 t/t2012-checkout-last.sh                 |  1 +
 t/t2018-checkout-branch.sh               |  1 +
 t/t2025-checkout-no-overlay.sh           |  1 +
 t/t3009-ls-files-others-nonsubmodule.sh  |  1 +
 t/t3010-ls-files-killed-modified.sh      |  2 ++
 t/t3050-subprojects-fetch.sh             |  1 +
 t/t3060-ls-files-with-tree.sh            |  2 ++
 t/t3409-rebase-environ.sh                |  1 +
 t/t3413-rebase-hook.sh                   |  1 +
 t/t3428-rebase-signoff.sh                |  1 +
 t/t3429-rebase-edit-todo.sh              |  1 +
 t/t3433-rebase-across-mode-change.sh     |  1 +
 t/t4015-diff-whitespace.sh               |  4 +--
 t/t4045-diff-relative.sh                 |  2 ++
 t/t4052-stat-output.sh                   |  1 +
 t/t4053-diff-no-index.sh                 |  1 +
 t/t4067-diff-partial-clone.sh            |  1 +
 t/t4111-apply-subdir.sh                  |  1 +
 t/t4135-apply-weird-filenames.sh         |  1 +
 t/t4213-log-tabexpand.sh                 |  1 +
 t/t5544-pack-objects-hook.sh             |  2 ++
 t/t5554-noop-fetch-negotiator.sh         |  2 ++
 t/t5610-clone-detached.sh                |  1 +
 t/t5611-clone-config.sh                  |  1 +
 t/t5614-clone-submodules-shallow.sh      |  1 +
 t/t5617-clone-submodules-remote.sh       |  1 +
 t/t5618-alternate-refs.sh                |  2 ++
 t/t6060-merge-index.sh                   |  2 ++
 t/t6301-for-each-ref-errors.sh           |  1 +
 t/t6401-merge-criss-cross.sh             |  2 ++
 t/t6406-merge-attr.sh                    |  1 +
 t/t6407-merge-binary.sh                  |  1 +
 t/t6415-merge-dir-to-symlink.sh          |  1 +
 t/t6435-merge-sparse.sh                  |  1 +
 t/t7103-reset-bare.sh                    |  2 +-
 t/t7504-commit-msg-hook.sh               |  1 +
 t/t7517-per-repo-email.sh                |  1 +
 t/t7520-ignored-hook-warning.sh          |  1 +
 t/t7605-merge-resolve.sh                 |  1 +
 t/t7614-merge-signoff.sh                 |  1 +
 t/t9003-help-autocorrect.sh              |  2 ++
 t/t9115-git-svn-dcommit-funky-renames.sh |  1 -
 t/t9146-git-svn-empty-dirs.sh            |  1 -
 t/t9148-git-svn-propset.sh               |  1 -
 t/t9160-git-svn-preserve-empty-dirs.sh   |  1 -
 77 files changed, 142 insertions(+), 48 deletions(-)

Range-diff against v1:
 1:  7fe724599a7 !  1:  58a02e6bb4e tests: mark tests as passing with SANITIZE=leak
    @@ Commit message
           66eede4a37c (prepare_repo_settings(): plug leak of config values,
           2022-09-08)
     
    +    - t2012-checkout-last.sh, t7504-commit-msg-hook.sh,
    +      t91{15,46,60}-git-svn-*.sh: The in-flight "pw/rebase-no-reflog-action"
    +      series, upon which this is based:
    +      https://lore.kernel.org/git/pull.1405.git.1667575142.gitgitgadget@gmail.com/
    +
         Let's mark all of these as passing with
         "TEST_PASSES_SANITIZE_LEAK=true", to have it regression tested,
         including as part of the "linux-leaks" CI job.
    @@ t/t1022-read-tree-partial-clone.sh
      
      test_expect_success 'read-tree in partial clone prefetches in one batch' '
     
    + ## t/t2012-checkout-last.sh ##
    +@@ t/t2012-checkout-last.sh: test_description='checkout can switch to last branch and merge base'
    + 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/t4015-diff-whitespace.sh ##
     @@ t/t4015-diff-whitespace.sh: test_expect_success 'no effect on diff from --color-moved with --word-diff' '
      	test_cmp expect actual
    @@ t/t7103-reset-bare.sh: test_expect_success '"mixed" reset is not allowed in bare
      	git reset --soft HEAD^ &&
      	git show --pretty=format:%s >out &&
      	echo one >expect &&
    +
    + ## t/t7504-commit-msg-hook.sh ##
    +@@ t/t7504-commit-msg-hook.sh: test_description='commit-msg hook'
    + 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 'with no hook' '
    +
    + ## t/t9115-git-svn-dcommit-funky-renames.sh ##
    +@@
    + 
    + test_description='git svn dcommit can commit renames of files with ugly names'
    + 
    +-TEST_FAILS_SANITIZE_LEAK=true
    + . ./lib-git-svn.sh
    + 
    + test_expect_success 'load repository with strange names' '
    +
    + ## t/t9146-git-svn-empty-dirs.sh ##
    +@@
    + 
    + test_description='git svn creates empty directories'
    + 
    +-TEST_FAILS_SANITIZE_LEAK=true
    + . ./lib-git-svn.sh
    + 
    + test_expect_success 'initialize repo' '
    +
    + ## t/t9160-git-svn-preserve-empty-dirs.sh ##
    +@@ t/t9160-git-svn-preserve-empty-dirs.sh: This test uses git to clone a Subversion repository that contains empty
    + directories, and checks that corresponding directories are created in the
    + local Git repository with placeholder files.'
    + 
    +-TEST_FAILS_SANITIZE_LEAK=true
    + . ./lib-git-svn.sh
    + 
    + GIT_REPO=git-svn-repo
 2:  819fde89a24 =  2:  e9962ad790e {reset,merge}: call discard_index() before returning
 3:  c1003454939 =  3:  8e76a29cce7 commit: discard partial cache before (re-)reading it
 4:  68a390619b9 =  4:  24cefde07f0 read-cache.c: clear and free "sparse_checkout_patterns"
 5:  29123e62391 =  5:  f4fc08a9bc8 dir.c: free "ident" and "exclude_per_dir" in "struct untracked_cache"
 6:  009c41d2e91 !  6:  698a335b1a3 built-ins & libs & helpers: add/move destructors, fix leaks
    @@ builtin/checkout.c: static void die_if_some_operation_in_progress(void)
     
      ## builtin/rebase.c ##
     @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
    - 	strbuf_release(&buf);
      	strbuf_release(&revisions);
    + 	free(options.reflog_action);
      	free(options.head_name);
     +	strvec_clear(&options.git_am_opts);
      	free(options.gpg_sign_opt);
    @@ t/helper/test-fake-ssh.c: int cmd_main(int argc, const char **argv)
      		fprintf(f, "%s%s", i > 0 ? " " : "", i > 0 ? argv[i] : "ssh:");
      	fprintf(f, "\n");
     
    + ## t/t3409-rebase-environ.sh ##
    +@@
    + 
    + test_description='git rebase interactive environment'
    + 
    ++TEST_PASSES_SANITIZE_LEAK=true
    + . ./test-lib.sh
    + 
    + test_expect_success 'setup' '
    +
      ## t/t3428-rebase-signoff.sh ##
     @@ t/t3428-rebase-signoff.sh: test_description='git rebase --signoff
      This test runs git rebase --signoff and make sure that it works.
    @@ t/t3428-rebase-signoff.sh: test_description='git rebase --signoff
      . ./test-lib.sh
      
      # A simple file to commit
    +
    + ## t/t3433-rebase-across-mode-change.sh ##
    +@@
    + 
    + test_description='git rebase across mode change'
    + 
    ++TEST_PASSES_SANITIZE_LEAK=true
    + . ./test-lib.sh
    + 
    + test_expect_success 'setup' '
 7:  38e7c863dd8 =  7:  a2cc6f10d0d unpack-file: fix ancient leak in create_temp_file()
 8:  2944aaa3ddc !  8:  45aa6e24c66 revision API: call graph_clear() in release_revisions()
    @@ revision.c: void release_revisions(struct rev_info *revs)
      	diff_free(&revs->pruning);
      	reflog_walk_info_release(revs->reflog_info);
     
    + ## t/t3413-rebase-hook.sh ##
    +@@ t/t3413-rebase-hook.sh: test_description='git rebase with its hook(s)'
    + 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/t4052-stat-output.sh ##
     @@ t/t4052-stat-output.sh: test_description='test --stat output of various commands'
      GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 9:  d592439760e =  9:  46809d9be80 ls-files: fix a --with-tree memory leak
10:  9c70bfa334e ! 10:  243ab74120b sequencer.c: fix "opts->strategy" leak in read_strategy_opts()
    @@ Commit message
     
         When "read_strategy_opts()" is called we may have populated the
         "opts->strategy" before, so we'll need to free() it to avoid leaking
    -    memory. Along with the preceding commit this change various
    -    rebase-related tests pass.
    +    memory.
    +
    +    We populate it before because we cal get_replay_opts() from within
    +    "rebase.c" with an already populated "opts", which we then copy. Then
    +    if we're doing a "rebase -i" the sequencer API itself will promptly
    +    clobber our alloc'd version of it with its own.
    +
    +    If this code is changed to do, instead of the added free() here a:
    +
    +            if (opts->strategy)
    +                    opts->strategy = xstrdup("another leak");
    +
    +    We get a couple of stacktraces from -fsanitize=leak showing how we
    +    ended up clobbering the already allocated value, i.e.:
    +
    +            Direct leak of 6 byte(s) in 1 object(s) allocated from:
    +                #0 0x7f2e8cd45545 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:75
    +                #1 0x7f2e8cb0fcaa in __GI___strdup string/strdup.c:42
    +                #2 0x6c4778 in xstrdup wrapper.c:39
    +                #3 0x66bcb8 in read_strategy_opts sequencer.c:2902
    +                #4 0x66bf7b in read_populate_opts sequencer.c:2969
    +                #5 0x6723f9 in sequencer_continue sequencer.c:5063
    +                #6 0x4a4f74 in run_sequencer_rebase builtin/rebase.c:348
    +                #7 0x4a64c8 in run_specific_rebase builtin/rebase.c:753
    +                #8 0x4a9b8b in cmd_rebase builtin/rebase.c:1824
    +                #9 0x407a32 in run_builtin git.c:466
    +                #10 0x407e0a in handle_builtin git.c:721
    +                #11 0x40803d in run_argv git.c:788
    +                #12 0x40850f in cmd_main git.c:923
    +                #13 0x4eee79 in main common-main.c:57
    +                #14 0x7f2e8ca9f209 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    +                #15 0x7f2e8ca9f2bb in __libc_start_main_impl ../csu/libc-start.c:389
    +                #16 0x405fd0 in _start (git+0x405fd0)
    +
    +            Direct leak of 4 byte(s) in 1 object(s) allocated from:
    +                #0 0x7f2e8cd45545 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:75
    +                #1 0x7f2e8cb0fcaa in __GI___strdup string/strdup.c:42
    +                #2 0x6c4778 in xstrdup wrapper.c:39
    +                #3 0x4a3c31 in xstrdup_or_null git-compat-util.h:1169
    +                #4 0x4a447a in get_replay_opts builtin/rebase.c:163
    +                #5 0x4a4f5b in run_sequencer_rebase builtin/rebase.c:346
    +                #6 0x4a64c8 in run_specific_rebase builtin/rebase.c:753
    +                #7 0x4a9b8b in cmd_rebase builtin/rebase.c:1824
    +                #8 0x407a32 in run_builtin git.c:466
    +                #9 0x407e0a in handle_builtin git.c:721
    +                #10 0x40803d in run_argv git.c:788
    +                #11 0x40850f in cmd_main git.c:923
    +                #12 0x4eee79 in main common-main.c:57
    +                #13 0x7f2e8ca9f209 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    +                #14 0x7f2e8ca9f2bb in __libc_start_main_impl ../csu/libc-start.c:389
    +                #15 0x405fd0 in _start (git+0x405fd0)
    +
    +    This can be seen in e.g. the 4th test of
    +    "t3404-rebase-interactive.sh".
    +
    +    In the larger picture the ownership of the "struct replay_opts" is
    +    quite a mess, e.g. in this case rebase.c's static "get_replay_opts()"
    +    function partially creates it, but nothing in rebase.c will free()
    +    it. The structure is "mostly owned" by the sequencer API, but it also
    +    expects to get these partially populated versions of it.
    +
    +    It would be better to have rebase keep track of what it allocated, and
    +    free() that, and to pass that as a "const" to the sequencer API, which
    +    would copy what it needs to its own version, and to free() that.
    +
    +    But doing so is a much larger change, and however messy the ownership
    +    boundary is here is consistent with what we're doing already, so let's
    +    just free() this to fix the leak.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
11:  c7722f68ae2 = 11:  15f0837697d connected.c: free the "struct packed_git"
12:  7429151b5c4 <  -:  ----------- sequencer.c: fix a pick_commits() leak
13:  fda9914b558 = 12:  f6d76250174 rebase: don't leak on "--abort"
14:  02e1dddf149 <  -:  ----------- sequencer.c: fix sequencer_continue() leak
15:  c9f51abf0e2 = 13:  10a477c7730 cherry-pick: free "struct replay_opts" members
16:  f7d39020a7c = 14:  614c8fc1aef revert: fix parse_options_concat() leak
17:  33a5753cc3a = 15:  f3e6a030394 built-ins: use free() not UNLEAK() if trivial, rm dead code
-- 
2.38.0.1467.g709fbdff1a9

[PATCH v2 01/15] tests: mark tests as passing with SANITIZE=leak

[Ævar Arnfjörð Bjarmason]

This marks tests that have been leak-free since various recent
commits, but which were not marked us such when the memory leak was
fixed. These were mostly discovered with the "check" mode added in
faececa53f9 (test-lib: have the "check" mode for SANITIZE=leak
consider leak logs, 2022-07-28).

Commits that fixed the last memory leak in these tests. Per narrowing
down when they started to pass under SANITIZE=leak with "bisect":

- t1022-read-tree-partial-clone.sh:
  7e2619d8ff0 (list_objects_filter_options: plug leak of filter_spec
  strings, 2022-09-08)

- t4053-diff-no-index.sh: 07a6f94a6d0 (diff-no-index: release prefixed
  filenames, 2022-09-07)

- t6415-merge-dir-to-symlink.sh: bac92b1f39f (Merge branch
  'js/ort-clean-up-after-failed-merge', 2022-08-08).

- t5554-noop-fetch-negotiator.sh:
  66eede4a37c (prepare_repo_settings(): plug leak of config values,
  2022-09-08)

- t2012-checkout-last.sh, t7504-commit-msg-hook.sh,
  t91{15,46,60}-git-svn-*.sh: The in-flight "pw/rebase-no-reflog-action"
  series, upon which this is based:
  https://lore.kernel.org/git/pull.1405.git.1667575142.gitgitgadget@gmail.com/

Let's mark all of these as passing with
"TEST_PASSES_SANITIZE_LEAK=true", to have it regression tested,
including as part of the "linux-leaks" CI job.

Additionally, let's remove the "!SANITIZE_LEAK" prerequisite from
tests that now pass, these were marked as failing in:

- 77e56d55ba6 (diff.c: fix a double-free regression in a18d66cefb,
  2022-03-17)
- c4d1d526312 (tests: change some 'test $(git) = "x"' to test_cmp,
  2022-03-07)

These were not spotted with the new "check" mode, but manually, it
doesn't cover these sort of prerequisites. There's few enough that we
shouldn't bother to automate it. They'll be going away sooner than
later.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1022-read-tree-partial-clone.sh       | 2 +-
 t/t2012-checkout-last.sh                 | 1 +
 t/t4015-diff-whitespace.sh               | 4 ++--
 t/t4053-diff-no-index.sh                 | 1 +
 t/t5554-noop-fetch-negotiator.sh         | 2 ++
 t/t6415-merge-dir-to-symlink.sh          | 1 +
 t/t7103-reset-bare.sh                    | 2 +-
 t/t7504-commit-msg-hook.sh               | 1 +
 t/t9115-git-svn-dcommit-funky-renames.sh | 1 -
 t/t9146-git-svn-empty-dirs.sh            | 1 -
 t/t9160-git-svn-preserve-empty-dirs.sh   | 1 -
 11 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/t/t1022-read-tree-partial-clone.sh b/t/t1022-read-tree-partial-clone.sh
index a9953b6a71c..d08563de5c9 100755
--- a/t/t1022-read-tree-partial-clone.sh
+++ b/t/t1022-read-tree-partial-clone.sh
@@ -3,7 +3,7 @@
 test_description='git read-tree in partial clones'
 
 TEST_NO_CREATE_REPO=1
-
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'read-tree in partial clone prefetches in one batch' '
diff --git a/t/t2012-checkout-last.sh b/t/t2012-checkout-last.sh
index 1f6c4ed0428..4b6372f4c3e 100755
--- a/t/t2012-checkout-last.sh
+++ b/t/t2012-checkout-last.sh
@@ -5,6 +5,7 @@ test_description='checkout can switch to last branch and merge base'
 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' '
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index f3e20dd5bba..b298f220e01 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -1638,7 +1638,7 @@ test_expect_success 'no effect on diff from --color-moved with --word-diff' '
 	test_cmp expect actual
 '
 
-test_expect_success !SANITIZE_LEAK 'no effect on show from --color-moved with --word-diff' '
+test_expect_success 'no effect on show from --color-moved with --word-diff' '
 	git show --color-moved --word-diff >actual &&
 	git show --word-diff >expect &&
 	test_cmp expect actual
@@ -2024,7 +2024,7 @@ test_expect_success '--color-moved rewinds for MIN_ALNUM_COUNT' '
 	test_cmp expected actual
 '
 
-test_expect_success !SANITIZE_LEAK 'move detection with submodules' '
+test_expect_success 'move detection with submodules' '
 	test_create_repo bananas &&
 	echo ripe >bananas/recipe &&
 	git -C bananas add recipe &&
diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh
index 3feadf0e359..4e9fa0403d3 100755
--- a/t/t4053-diff-no-index.sh
+++ b/t/t4053-diff-no-index.sh
@@ -2,6 +2,7 @@
 
 test_description='diff --no-index'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t5554-noop-fetch-negotiator.sh b/t/t5554-noop-fetch-negotiator.sh
index 2ac7b5859e7..06991e8e8aa 100755
--- a/t/t5554-noop-fetch-negotiator.sh
+++ b/t/t5554-noop-fetch-negotiator.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='test noop fetch negotiator'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'noop negotiator does not emit any "have"' '
diff --git a/t/t6415-merge-dir-to-symlink.sh b/t/t6415-merge-dir-to-symlink.sh
index 2655e295f5a..ae00492c768 100755
--- a/t/t6415-merge-dir-to-symlink.sh
+++ b/t/t6415-merge-dir-to-symlink.sh
@@ -4,6 +4,7 @@ test_description='merging when a directory was replaced with a symlink'
 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 'create a commit where dir a/b changed to symlink' '
diff --git a/t/t7103-reset-bare.sh b/t/t7103-reset-bare.sh
index a60153f9f32..18bbd9975eb 100755
--- a/t/t7103-reset-bare.sh
+++ b/t/t7103-reset-bare.sh
@@ -63,7 +63,7 @@ test_expect_success '"mixed" reset is not allowed in bare' '
 	test_must_fail git reset --mixed HEAD^
 '
 
-test_expect_success !SANITIZE_LEAK '"soft" reset is allowed in bare' '
+test_expect_success '"soft" reset is allowed in bare' '
 	git reset --soft HEAD^ &&
 	git show --pretty=format:%s >out &&
 	echo one >expect &&
diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh
index a39de8c1126..07ca46fb0d5 100755
--- a/t/t7504-commit-msg-hook.sh
+++ b/t/t7504-commit-msg-hook.sh
@@ -5,6 +5,7 @@ test_description='commit-msg hook'
 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 'with no hook' '
diff --git a/t/t9115-git-svn-dcommit-funky-renames.sh b/t/t9115-git-svn-dcommit-funky-renames.sh
index 419f055721d..743fbe1fe46 100755
--- a/t/t9115-git-svn-dcommit-funky-renames.sh
+++ b/t/t9115-git-svn-dcommit-funky-renames.sh
@@ -5,7 +5,6 @@
 
 test_description='git svn dcommit can commit renames of files with ugly names'
 
-TEST_FAILS_SANITIZE_LEAK=true
 . ./lib-git-svn.sh
 
 test_expect_success 'load repository with strange names' '
diff --git a/t/t9146-git-svn-empty-dirs.sh b/t/t9146-git-svn-empty-dirs.sh
index 79c26ed69c1..09606f1b3cf 100755
--- a/t/t9146-git-svn-empty-dirs.sh
+++ b/t/t9146-git-svn-empty-dirs.sh
@@ -4,7 +4,6 @@
 
 test_description='git svn creates empty directories'
 
-TEST_FAILS_SANITIZE_LEAK=true
 . ./lib-git-svn.sh
 
 test_expect_success 'initialize repo' '
diff --git a/t/t9160-git-svn-preserve-empty-dirs.sh b/t/t9160-git-svn-preserve-empty-dirs.sh
index 9cf7a1427ab..36c6b1a12ff 100755
--- a/t/t9160-git-svn-preserve-empty-dirs.sh
+++ b/t/t9160-git-svn-preserve-empty-dirs.sh
@@ -9,7 +9,6 @@ This test uses git to clone a Subversion repository that contains empty
 directories, and checks that corresponding directories are created in the
 local Git repository with placeholder files.'
 
-TEST_FAILS_SANITIZE_LEAK=true
 . ./lib-git-svn.sh
 
 GIT_REPO=git-svn-repo
-- 
2.38.0.1467.g709fbdff1a9

[PATCH v2 02/15] {reset,merge}: call discard_index() before returning

[Ævar Arnfjörð Bjarmason]

These two built-ins both deal with the index, but weren't discarding
it. In subsequent commits we'll add more free()-ing to discard_index()
that we've missed, but let's first call the existing function.

We can doubtless add discard_index() (or its alias discard_cache()) to
a lot more places, but let's just add it here for now.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/merge.c | 1 +
 builtin/reset.c | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/builtin/merge.c b/builtin/merge.c
index 5900b81729d..a6698adbfd3 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1794,5 +1794,6 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	}
 	strbuf_release(&buf);
 	free(branch_to_free);
+	discard_index(&the_index);
 	return ret;
 }
diff --git a/builtin/reset.c b/builtin/reset.c
index fdce6f8c856..69f18a248ee 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -481,5 +481,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	if (!pathspec.nr)
 		remove_branch_state(the_repository, 0);
 
+	discard_index(&the_index);
+
 	return update_ref_status;
 }
-- 
2.38.0.1467.g709fbdff1a9

[PATCH v2 03/15] commit: discard partial cache before (re-)reading it

[Ævar Arnfjörð Bjarmason]

The read_cache() in prepare_to_commit() would end up clobbering the
pointer we had for a previously populated "the_index.cache_tree" in
the very common case of "git commit" stressed by e.g. the tests being
changed here.

We'd populate "the_index.cache_tree" by calling
"update_main_cache_tree" in prepare_index(), but would not end up with
a "fully prepared" index. What constitutes an existing index is
clearly overly fuzzy, here we'll check "active_nr" (aka
"the_index.cache_nr"), but our "the_index.cache_tree" might have been
malloc()'d already.

Thus the code added in 11c8a74a64a (commit: write cache-tree data when
writing index anyway, 2011-12-06) would end up allocating the
"cache_tree", and would interact here with code added in
7168624c353 (Do not generate full commit log message if it is not
going to be used, 2007-11-28). The result was a very common memory
leak.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/commit.c                        | 7 +++++--
 t/t0068-for-each-repo.sh                | 1 +
 t/t0070-fundamental.sh                  | 1 +
 t/t1404-update-ref-errors.sh            | 2 ++
 t/t1409-avoid-packing-refs.sh           | 1 +
 t/t1413-reflog-detach.sh                | 1 +
 t/t1501-work-tree.sh                    | 2 ++
 t/t2025-checkout-no-overlay.sh          | 1 +
 t/t3009-ls-files-others-nonsubmodule.sh | 1 +
 t/t3010-ls-files-killed-modified.sh     | 2 ++
 t/t4045-diff-relative.sh                | 2 ++
 t/t4111-apply-subdir.sh                 | 1 +
 t/t4135-apply-weird-filenames.sh        | 1 +
 t/t4213-log-tabexpand.sh                | 1 +
 t/t5618-alternate-refs.sh               | 2 ++
 t/t6301-for-each-ref-errors.sh          | 1 +
 t/t7520-ignored-hook-warning.sh         | 1 +
 t/t7614-merge-signoff.sh                | 1 +
 t/t9003-help-autocorrect.sh             | 2 ++
 19 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index e22bdf23f5f..c291199b704 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -987,8 +987,11 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		struct object_id oid;
 		const char *parent = "HEAD";
 
-		if (!active_nr && read_cache() < 0)
-			die(_("Cannot read index"));
+		if (!active_nr) {
+			discard_cache();
+			if (read_cache() < 0)
+				die(_("Cannot read index"));
+		}
 
 		if (amend)
 			parent = "HEAD^1";
diff --git a/t/t0068-for-each-repo.sh b/t/t0068-for-each-repo.sh
index 4675e852517..9f63f612425 100755
--- a/t/t0068-for-each-repo.sh
+++ b/t/t0068-for-each-repo.sh
@@ -2,6 +2,7 @@
 
 test_description='git for-each-repo builtin'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'run based on configured value' '
diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index 8d59905ef09..574de341980 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -6,6 +6,7 @@ test_description='check that the most basic functions work
 Verify wrappers and compatibility functions.
 '
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'character classes (isspace, isalpha etc.)' '
diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh
index 13c2b43bbae..b5606d93b52 100755
--- a/t/t1404-update-ref-errors.sh
+++ b/t/t1404-update-ref-errors.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='Test git update-ref error handling'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Create some references, perhaps run pack-refs --all, then try to
diff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh
index be12fb63506..f23c0152a85 100755
--- a/t/t1409-avoid-packing-refs.sh
+++ b/t/t1409-avoid-packing-refs.sh
@@ -2,6 +2,7 @@
 
 test_description='avoid rewriting packed-refs unnecessarily'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Add an identifying mark to the packed-refs file header line. This
diff --git a/t/t1413-reflog-detach.sh b/t/t1413-reflog-detach.sh
index 934688a1ee8..d2a4822d46f 100755
--- a/t/t1413-reflog-detach.sh
+++ b/t/t1413-reflog-detach.sh
@@ -4,6 +4,7 @@ test_description='Test reflog interaction with detached HEAD'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 reset_state () {
diff --git a/t/t1501-work-tree.sh b/t/t1501-work-tree.sh
index b75558040ff..ae6528aecea 100755
--- a/t/t1501-work-tree.sh
+++ b/t/t1501-work-tree.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='test separate work tree'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t2025-checkout-no-overlay.sh b/t/t2025-checkout-no-overlay.sh
index 8f13341cf8e..3832c3de813 100755
--- a/t/t2025-checkout-no-overlay.sh
+++ b/t/t2025-checkout-no-overlay.sh
@@ -2,6 +2,7 @@
 
 test_description='checkout --no-overlay <tree-ish> -- <pathspec>'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t3009-ls-files-others-nonsubmodule.sh b/t/t3009-ls-files-others-nonsubmodule.sh
index 963f3462b75..14218b34243 100755
--- a/t/t3009-ls-files-others-nonsubmodule.sh
+++ b/t/t3009-ls-files-others-nonsubmodule.sh
@@ -18,6 +18,7 @@ This test runs git ls-files --others with the following working tree:
       git repository with a commit and an untracked file
 '
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup: directories' '
diff --git a/t/t3010-ls-files-killed-modified.sh b/t/t3010-ls-files-killed-modified.sh
index 580e158f991..054178703d7 100755
--- a/t/t3010-ls-files-killed-modified.sh
+++ b/t/t3010-ls-files-killed-modified.sh
@@ -41,6 +41,8 @@ Also for modification test, the cache and working tree have:
 We should report path0, path1, path2/file2, path3/file3, path7 and path8
 modified without reporting path9 and path10.  submod1 is also modified.
 '
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'git update-index --add to add various paths.' '
diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh
index fab351b48a1..198dfc91906 100755
--- a/t/t4045-diff-relative.sh
+++ b/t/t4045-diff-relative.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='diff --relative tests'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t4111-apply-subdir.sh b/t/t4111-apply-subdir.sh
index 1618a6dbc7c..e9a87d761d1 100755
--- a/t/t4111-apply-subdir.sh
+++ b/t/t4111-apply-subdir.sh
@@ -2,6 +2,7 @@
 
 test_description='patching from inconvenient places'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t4135-apply-weird-filenames.sh b/t/t4135-apply-weird-filenames.sh
index 6bc3fb97a75..d3502c6fddf 100755
--- a/t/t4135-apply-weird-filenames.sh
+++ b/t/t4135-apply-weird-filenames.sh
@@ -2,6 +2,7 @@
 
 test_description='git apply with weird postimage filenames'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t4213-log-tabexpand.sh b/t/t4213-log-tabexpand.sh
index 53a4af32449..590fce95e90 100755
--- a/t/t4213-log-tabexpand.sh
+++ b/t/t4213-log-tabexpand.sh
@@ -2,6 +2,7 @@
 
 test_description='log/show --expand-tabs'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 HT="	"
diff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh
index 3353216f09e..f905db0a3fd 100755
--- a/t/t5618-alternate-refs.sh
+++ b/t/t5618-alternate-refs.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='test handling of --alternate-refs traversal'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Avoid test_commit because we want a specific and known set of refs:
diff --git a/t/t6301-for-each-ref-errors.sh b/t/t6301-for-each-ref-errors.sh
index 40edf9dab53..bfda1f46ad2 100755
--- a/t/t6301-for-each-ref-errors.sh
+++ b/t/t6301-for-each-ref-errors.sh
@@ -2,6 +2,7 @@
 
 test_description='for-each-ref errors for broken refs'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 ZEROS=$ZERO_OID
diff --git a/t/t7520-ignored-hook-warning.sh b/t/t7520-ignored-hook-warning.sh
index dc57526e6f1..184b2589893 100755
--- a/t/t7520-ignored-hook-warning.sh
+++ b/t/t7520-ignored-hook-warning.sh
@@ -2,6 +2,7 @@
 
 test_description='ignored hook warning'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '
diff --git a/t/t7614-merge-signoff.sh b/t/t7614-merge-signoff.sh
index fee258d4f0c..cf96a35e8e7 100755
--- a/t/t7614-merge-signoff.sh
+++ b/t/t7614-merge-signoff.sh
@@ -8,6 +8,7 @@ This test runs git merge --signoff and makes sure that it works.
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Setup test files
diff --git a/t/t9003-help-autocorrect.sh b/t/t9003-help-autocorrect.sh
index f00deaf3815..4b9cb4c942f 100755
--- a/t/t9003-help-autocorrect.sh
+++ b/t/t9003-help-autocorrect.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='help.autocorrect finding a match'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-- 
2.38.0.1467.g709fbdff1a9

[PATCH v2 04/15] read-cache.c: clear and free "sparse_checkout_patterns"

[Ævar Arnfjörð Bjarmason]

The "sparse_checkout_patterns" member was added to the "struct
index_state" in 836e25c51b2 (sparse-checkout: hold pattern list in
index, 2021-03-30), but wasn't added to discard_index(). Let's do
that.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 read-cache.c                         | 5 +++++
 t/t1011-read-tree-sparse-checkout.sh | 1 +
 t/t2018-checkout-branch.sh           | 1 +
 t/t6435-merge-sparse.sh              | 1 +
 4 files changed, 8 insertions(+)

diff --git a/read-cache.c b/read-cache.c
index 32024029274..7c6477487a7 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2558,6 +2558,11 @@ int discard_index(struct index_state *istate)
 	free_untracked_cache(istate->untracked);
 	istate->untracked = NULL;
 
+	if (istate->sparse_checkout_patterns) {
+		clear_pattern_list(istate->sparse_checkout_patterns);
+		FREE_AND_NULL(istate->sparse_checkout_patterns);
+	}
+
 	if (istate->ce_mem_pool) {
 		mem_pool_discard(istate->ce_mem_pool, should_validate_cache_entries());
 		FREE_AND_NULL(istate->ce_mem_pool);
diff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh
index 742f0fa909f..595b24c0adb 100755
--- a/t/t1011-read-tree-sparse-checkout.sh
+++ b/t/t1011-read-tree-sparse-checkout.sh
@@ -12,6 +12,7 @@ test_description='sparse checkout tests
 '
 
 TEST_CREATE_REPO_NO_TEMPLATE=1
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-read-tree.sh
 
diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
index 771c3c3c50e..8581ad34379 100755
--- a/t/t2018-checkout-branch.sh
+++ b/t/t2018-checkout-branch.sh
@@ -3,6 +3,7 @@
 test_description='checkout'
 
 TEST_CREATE_REPO_NO_TEMPLATE=1
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Arguments: [!] <branch> <oid> [<checkout options>]
diff --git a/t/t6435-merge-sparse.sh b/t/t6435-merge-sparse.sh
index fde4aa3cd1a..78628fb248a 100755
--- a/t/t6435-merge-sparse.sh
+++ b/t/t6435-merge-sparse.sh
@@ -3,6 +3,7 @@
 test_description='merge with sparse files'
 
 TEST_CREATE_REPO_NO_TEMPLATE=1
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # test_file $filename $content
-- 
2.38.0.1467.g709fbdff1a9

[PATCH v2 05/15] dir.c: free "ident" and "exclude_per_dir" in "struct untracked_cache"

[Ævar Arnfjörð Bjarmason]

When the "ident" member of the structure was added in
1e8fef609e7 (untracked cache: guard and disable on system changes,
2015-03-08) this function wasn't updated to free it. Let's do so.

Let's also free the "exclude_per_dir" memory we've been leaking
since[1], while making sure not to free() the constant ".gitignore"
string we add by default[2].

As we now have three struct members we're freeing let's change
free_untracked_cache() to return early if "uc" isn't defined. We won't
hand it to free() now, but that was just for convenience, once we're
dealing with >=2 struct members this pattern is more convenient.

1. f9e6c649589 (untracked cache: load from UNTR index extension,
   2015-03-08)
2. 039bc64e886 (core.excludesfile clean-up, 2007-11-14)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 dir.c | 10 +++++++---
 dir.h |  1 +
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/dir.c b/dir.c
index d604d1bab98..fbdb24fc819 100644
--- a/dir.c
+++ b/dir.c
@@ -3581,8 +3581,12 @@ static void free_untracked(struct untracked_cache_dir *ucd)
 
 void free_untracked_cache(struct untracked_cache *uc)
 {
-	if (uc)
-		free_untracked(uc->root);
+	if (!uc)
+		return;
+
+	free(uc->exclude_per_dir_to_free);
+	strbuf_release(&uc->ident);
+	free_untracked(uc->root);
 	free(uc);
 }
 
@@ -3739,7 +3743,7 @@ struct untracked_cache *read_untracked_extension(const void *data, unsigned long
 		      next + offset + hashsz);
 	uc->dir_flags = get_be32(next + ouc_offset(dir_flags));
 	exclude_per_dir = (const char *)next + exclude_per_dir_offset;
-	uc->exclude_per_dir = xstrdup(exclude_per_dir);
+	uc->exclude_per_dir = uc->exclude_per_dir_to_free = xstrdup(exclude_per_dir);
 	/* NUL after exclude_per_dir is covered by sizeof(*ouc) */
 	next += exclude_per_dir_offset + strlen(exclude_per_dir) + 1;
 	if (next >= end)
diff --git a/dir.h b/dir.h
index 674747d93af..8acfc044181 100644
--- a/dir.h
+++ b/dir.h
@@ -188,6 +188,7 @@ struct untracked_cache {
 	struct oid_stat ss_info_exclude;
 	struct oid_stat ss_excludes_file;
 	const char *exclude_per_dir;
+	char *exclude_per_dir_to_free;
 	struct strbuf ident;
 	/*
 	 * dir_struct#flags must match dir_flags or the untracked
-- 
2.38.0.1467.g709fbdff1a9

[PATCH v2 06/15] built-ins & libs & helpers: add/move destructors, fix leaks

[Ævar Arnfjörð Bjarmason]

Fix various leaks in built-ins, libraries and a test helper here we
were missing a call to strbuf_release(), string_list_clear() etc, or
were calling them after a potential "return".

Comments on individual changes:

- builtin/checkout.c: Fix a memory leak that was introduced in [1]. A
  sibling leak introduced in [2] was recently fixed in [3]. As with [3]
  we should be using the wt_status_state_free_buffers() API introduced
  in [4].

- builtin/repack.c: Fix a leak that's been here since this use of
  "strbuf_release()" was added in a1bbc6c0176 (repack: rewrite the shell
  script in C, 2013-09-15). We don't use the variable for anything
  except this loop, so we can instead free it right afterwards.

- builtin/rev-parse: Fix a leak that's been here since this code was
  added in 21d47835386 (Add a parseopt mode to git-rev-parse to bring
  parse-options to shell scripts., 2007-11-04).

- builtin/stash.c: Fix a couple of leaks that have been here since
  this code was added in d4788af875c (stash: convert create to builtin,
  2019-02-25), we strbuf_release()'d only some of the "struct strbuf" we
  allocated earlier in the function, let's release all of them.

- ref-filter.c: Fix a leak in 482c1191869 (gpg-interface: improve
  interface for parsing tags, 2021-02-11), we don't use the "payload"
  variable that we ask parse_signature() to populate for us, so let's
  free it.

- t/helper/test-fake-ssh.c: Fix a leak that's been here since this
  code was added in 3064d5a38c7 (mingw: fix t5601-clone.sh,
  2016-01-27). Let's free the "struct strbuf" as soon as we don't need
  it anymore.

1. c45f0f525de (switch: reject if some operation is in progress,
   2019-03-29)
2. 2708ce62d21 (branch: sort detached HEAD based on a flag,
   2021-01-07)
3. abcac2e19fa (ref-filter.c: fix a leak in get_head_description,
   2022-09-25)
4. 962dd7ebc3e (wt-status: introduce wt_status_state_free_buffers(),
   2020-09-27).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/checkout.c                   | 2 ++
 builtin/rebase.c                     | 3 +++
 builtin/repack.c                     | 2 +-
 builtin/rev-parse.c                  | 1 +
 builtin/stash.c                      | 2 ++
 ref-filter.c                         | 1 +
 t/helper/test-fake-ssh.c             | 1 +
 t/t3409-rebase-environ.sh            | 1 +
 t/t3428-rebase-signoff.sh            | 1 +
 t/t3433-rebase-across-mode-change.sh | 1 +
 10 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 2a132392fbe..659dd5c4309 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1470,6 +1470,8 @@ static void die_if_some_operation_in_progress(void)
 		      "or \"git worktree add\"."));
 	if (state.bisect_in_progress)
 		warning(_("you are switching branch while bisecting"));
+
+	wt_status_state_free_buffers(&state);
 }
 
 static int checkout_branch(struct checkout_opts *opts,
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 4d6839a5785..6a831320313 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1828,10 +1828,13 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	strbuf_release(&revisions);
 	free(options.reflog_action);
 	free(options.head_name);
+	strvec_clear(&options.git_am_opts);
 	free(options.gpg_sign_opt);
 	free(options.cmd);
 	free(options.strategy);
 	strbuf_release(&options.git_format_patch_opt);
 	free(squash_onto_name);
+	string_list_clear(&exec, 0);
+	string_list_clear(&strategy_options, 0);
 	return !!ret;
 }
diff --git a/builtin/repack.c b/builtin/repack.c
index 10e23f9ee1f..fb3ac197426 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -956,6 +956,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 		item = string_list_append(&names, line.buf);
 		item->util = populate_pack_exts(item->string);
 	}
+	strbuf_release(&line);
 	fclose(out);
 	ret = finish_command(&cmd);
 	if (ret)
@@ -1124,7 +1125,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	string_list_clear(&existing_nonkept_packs, 0);
 	string_list_clear(&existing_kept_packs, 0);
 	clear_pack_geometry(geometry);
-	strbuf_release(&line);
 
 	return 0;
 }
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 8f61050bde8..e0244d63de6 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -530,6 +530,7 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)
 	strbuf_addstr(&parsed, " --");
 	sq_quote_argv(&parsed, argv);
 	puts(parsed.buf);
+	strbuf_release(&parsed);
 	return 0;
 }
 
diff --git a/builtin/stash.c b/builtin/stash.c
index bb5485b4095..8a64d564a19 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1686,8 +1686,10 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
 	}
 
 done:
+	strbuf_release(&patch);
 	free_stash_info(&info);
 	strbuf_release(&stash_msg_buf);
+	strbuf_release(&untracked_files);
 	return ret;
 }
 
diff --git a/ref-filter.c b/ref-filter.c
index 914908fac52..b40b76c3806 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1358,6 +1358,7 @@ static void find_subpos(const char *buf,
 
 	/* parse signature first; we might not even have a subject line */
 	parse_signature(buf, end - buf, &payload, &signature);
+	strbuf_release(&payload);
 
 	/* skip past header until we hit empty line */
 	while (*buf && *buf != '\n') {
diff --git a/t/helper/test-fake-ssh.c b/t/helper/test-fake-ssh.c
index 12beee99ad2..d5e511633ab 100644
--- a/t/helper/test-fake-ssh.c
+++ b/t/helper/test-fake-ssh.c
@@ -17,6 +17,7 @@ int cmd_main(int argc, const char **argv)
 	f = fopen(buf.buf, "w");
 	if (!f)
 		die("Could not write to %s", buf.buf);
+	strbuf_release(&buf);
 	for (i = 0; i < argc; i++)
 		fprintf(f, "%s%s", i > 0 ? " " : "", i > 0 ? argv[i] : "ssh:");
 	fprintf(f, "\n");
diff --git a/t/t3409-rebase-environ.sh b/t/t3409-rebase-environ.sh
index 83ffb39d9ff..acaf5558dbe 100755
--- a/t/t3409-rebase-environ.sh
+++ b/t/t3409-rebase-environ.sh
@@ -2,6 +2,7 @@
 
 test_description='git rebase interactive environment'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t3428-rebase-signoff.sh b/t/t3428-rebase-signoff.sh
index f6993b7e14d..e1b1e947647 100755
--- a/t/t3428-rebase-signoff.sh
+++ b/t/t3428-rebase-signoff.sh
@@ -5,6 +5,7 @@ test_description='git rebase --signoff
 This test runs git rebase --signoff and make sure that it works.
 '
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # A simple file to commit
diff --git a/t/t3433-rebase-across-mode-change.sh b/t/t3433-rebase-across-mode-change.sh
index 05df964670f..c8172b08522 100755
--- a/t/t3433-rebase-across-mode-change.sh
+++ b/t/t3433-rebase-across-mode-change.sh
@@ -2,6 +2,7 @@
 
 test_description='git rebase across mode change'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-- 
2.38.0.1467.g709fbdff1a9

[PATCH v2 07/15] unpack-file: fix ancient leak in create_temp_file()

[Ævar Arnfjörð Bjarmason]

Fix a leak that's been with us since 3407bb4940c (Add "unpack-file"
helper that unpacks a sha1 blob into a tmpfile., 2005-04-18). See
00c8fd493af (cat-file: use streaming API to print blobs, 2012-03-07)
for prior art which shows the same API pattern, i.e. free()-ing the
result of read_object_file() after it's used.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/unpack-file.c        | 1 +
 t/t6060-merge-index.sh       | 2 ++
 t/t6401-merge-criss-cross.sh | 2 ++
 t/t6406-merge-attr.sh        | 1 +
 t/t6407-merge-binary.sh      | 1 +
 t/t7605-merge-resolve.sh     | 1 +
 6 files changed, 8 insertions(+)

diff --git a/builtin/unpack-file.c b/builtin/unpack-file.c
index 9e8119dd354..88de32b7d7e 100644
--- a/builtin/unpack-file.c
+++ b/builtin/unpack-file.c
@@ -19,6 +19,7 @@ static char *create_temp_file(struct object_id *oid)
 	if (write_in_full(fd, buf, size) < 0)
 		die_errno("unable to write temp-file");
 	close(fd);
+	free(buf);
 	return path;
 }
 
diff --git a/t/t6060-merge-index.sh b/t/t6060-merge-index.sh
index ed449abe552..1a8b64cce18 100755
--- a/t/t6060-merge-index.sh
+++ b/t/t6060-merge-index.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='basic git merge-index / git-merge-one-file tests'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup diverging branches' '
diff --git a/t/t6401-merge-criss-cross.sh b/t/t6401-merge-criss-cross.sh
index 9d5e992878f..1962310408b 100755
--- a/t/t6401-merge-criss-cross.sh
+++ b/t/t6401-merge-criss-cross.sh
@@ -8,6 +8,8 @@
 
 
 test_description='Test criss-cross merge'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'prepare repository' '
diff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh
index 8650a88c40a..5e4e4dd6d9e 100755
--- a/t/t6406-merge-attr.sh
+++ b/t/t6406-merge-attr.sh
@@ -8,6 +8,7 @@ test_description='per path merge controlled by merge attribute'
 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 '
diff --git a/t/t6407-merge-binary.sh b/t/t6407-merge-binary.sh
index e8a28717cec..0753fc95f45 100755
--- a/t/t6407-merge-binary.sh
+++ b/t/t6407-merge-binary.sh
@@ -5,6 +5,7 @@ test_description='ask merge-recursive to merge binary files'
 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 '
diff --git a/t/t7605-merge-resolve.sh b/t/t7605-merge-resolve.sh
index 5d56c385464..62d935d31c2 100755
--- a/t/t7605-merge-resolve.sh
+++ b/t/t7605-merge-resolve.sh
@@ -4,6 +4,7 @@ test_description='git merge
 
 Testing the resolve strategy.'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-- 
2.38.0.1467.g709fbdff1a9

[PATCH v2 08/15] revision API: call graph_clear() in release_revisions()

[Ævar Arnfjörð Bjarmason]

Call graph_clear() in release_revisions(), this will free memory
allocated by e.g. this command, which will now run without memory
leaks:

	git -P log -1 --graph --no-graph --graph

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 revision.c             | 1 +
 t/t3413-rebase-hook.sh | 1 +
 t/t4052-stat-output.sh | 1 +
 3 files changed, 3 insertions(+)

diff --git a/revision.c b/revision.c
index 0760e78936e..650dd17599a 100644
--- a/revision.c
+++ b/revision.c
@@ -3020,6 +3020,7 @@ void release_revisions(struct rev_info *revs)
 	date_mode_release(&revs->date_mode);
 	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->pruning);
 	reflog_walk_info_release(revs->reflog_info);
diff --git a/t/t3413-rebase-hook.sh b/t/t3413-rebase-hook.sh
index 9fab0d779bb..e8456831e8b 100755
--- a/t/t3413-rebase-hook.sh
+++ b/t/t3413-rebase-hook.sh
@@ -5,6 +5,7 @@ test_description='git rebase with its hook(s)'
 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 '
diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh
index b5c281edaa7..3ee27e277dc 100755
--- a/t/t4052-stat-output.sh
+++ b/t/t4052-stat-output.sh
@@ -8,6 +8,7 @@ test_description='test --stat output of various commands'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-terminal.sh
 
-- 
2.38.0.1467.g709fbdff1a9

[PATCH v2 09/15] ls-files: fix a --with-tree memory leak

[Ævar Arnfjörð Bjarmason]

Fix a memory leak in overlay_tree_on_index(), we need to
clear_pathspec() at some point, which might as well be after the last
time we use it in the function.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/ls-files.c            | 1 +
 t/t3060-ls-files-with-tree.sh | 2 ++
 t/t9148-git-svn-propset.sh    | 1 -
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 4cf8a236483..a03b559ecaa 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -613,6 +613,7 @@ void overlay_tree_on_index(struct index_state *istate,
 	if (!fn)
 		fn = read_one_entry_quick;
 	err = read_tree(the_repository, tree, &pathspec, fn, istate);
+	clear_pathspec(&pathspec);
 	if (err)
 		die("unable to read tree entries %s", tree_name);
 
diff --git a/t/t3060-ls-files-with-tree.sh b/t/t3060-ls-files-with-tree.sh
index 52f76f7b57f..c4a72ae4462 100755
--- a/t/t3060-ls-files-with-tree.sh
+++ b/t/t3060-ls-files-with-tree.sh
@@ -8,6 +8,8 @@ test_description='git ls-files test (--with-tree).
 This test runs git ls-files --with-tree and in particular in
 a scenario known to trigger a crash with some versions of git.
 '
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t9148-git-svn-propset.sh b/t/t9148-git-svn-propset.sh
index 6cc76a07b39..aebb28995e5 100755
--- a/t/t9148-git-svn-propset.sh
+++ b/t/t9148-git-svn-propset.sh
@@ -5,7 +5,6 @@
 
 test_description='git svn propset tests'
 
-TEST_FAILS_SANITIZE_LEAK=true
 . ./lib-git-svn.sh
 
 test_expect_success 'setup propset via import' '
-- 
2.38.0.1467.g709fbdff1a9

[PATCH v2 10/15] sequencer.c: fix "opts->strategy" leak in read_strategy_opts()

[Ævar Arnfjörð Bjarmason]

When "read_strategy_opts()" is called we may have populated the
"opts->strategy" before, so we'll need to free() it to avoid leaking
memory.

We populate it before because we cal get_replay_opts() from within
"rebase.c" with an already populated "opts", which we then copy. Then
if we're doing a "rebase -i" the sequencer API itself will promptly
clobber our alloc'd version of it with its own.

If this code is changed to do, instead of the added free() here a:

	if (opts->strategy)
		opts->strategy = xstrdup("another leak");

We get a couple of stacktraces from -fsanitize=leak showing how we
ended up clobbering the already allocated value, i.e.:

	Direct leak of 6 byte(s) in 1 object(s) allocated from:
	    #0 0x7f2e8cd45545 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:75
	    #1 0x7f2e8cb0fcaa in __GI___strdup string/strdup.c:42
	    #2 0x6c4778 in xstrdup wrapper.c:39
	    #3 0x66bcb8 in read_strategy_opts sequencer.c:2902
	    #4 0x66bf7b in read_populate_opts sequencer.c:2969
	    #5 0x6723f9 in sequencer_continue sequencer.c:5063
	    #6 0x4a4f74 in run_sequencer_rebase builtin/rebase.c:348
	    #7 0x4a64c8 in run_specific_rebase builtin/rebase.c:753
	    #8 0x4a9b8b in cmd_rebase builtin/rebase.c:1824
	    #9 0x407a32 in run_builtin git.c:466
	    #10 0x407e0a in handle_builtin git.c:721
	    #11 0x40803d in run_argv git.c:788
	    #12 0x40850f in cmd_main git.c:923
	    #13 0x4eee79 in main common-main.c:57
	    #14 0x7f2e8ca9f209 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
	    #15 0x7f2e8ca9f2bb in __libc_start_main_impl ../csu/libc-start.c:389
	    #16 0x405fd0 in _start (git+0x405fd0)

	Direct leak of 4 byte(s) in 1 object(s) allocated from:
	    #0 0x7f2e8cd45545 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:75
	    #1 0x7f2e8cb0fcaa in __GI___strdup string/strdup.c:42
	    #2 0x6c4778 in xstrdup wrapper.c:39
	    #3 0x4a3c31 in xstrdup_or_null git-compat-util.h:1169
	    #4 0x4a447a in get_replay_opts builtin/rebase.c:163
	    #5 0x4a4f5b in run_sequencer_rebase builtin/rebase.c:346
	    #6 0x4a64c8 in run_specific_rebase builtin/rebase.c:753
	    #7 0x4a9b8b in cmd_rebase builtin/rebase.c:1824
	    #8 0x407a32 in run_builtin git.c:466
	    #9 0x407e0a in handle_builtin git.c:721
	    #10 0x40803d in run_argv git.c:788
	    #11 0x40850f in cmd_main git.c:923
	    #12 0x4eee79 in main common-main.c:57
	    #13 0x7f2e8ca9f209 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
	    #14 0x7f2e8ca9f2bb in __libc_start_main_impl ../csu/libc-start.c:389
	    #15 0x405fd0 in _start (git+0x405fd0)

This can be seen in e.g. the 4th test of
"t3404-rebase-interactive.sh".

In the larger picture the ownership of the "struct replay_opts" is
quite a mess, e.g. in this case rebase.c's static "get_replay_opts()"
function partially creates it, but nothing in rebase.c will free()
it. The structure is "mostly owned" by the sequencer API, but it also
expects to get these partially populated versions of it.

It would be better to have rebase keep track of what it allocated, and
free() that, and to pass that as a "const" to the sequencer API, which
would copy what it needs to its own version, and to free() that.

But doing so is a much larger change, and however messy the ownership
boundary is here is consistent with what we're doing already, so let's
just free() this to fix the leak.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 sequencer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sequencer.c b/sequencer.c
index e23f6f0b718..433fec21562 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2897,6 +2897,7 @@ static void read_strategy_opts(struct replay_opts *opts, struct strbuf *buf)
 	strbuf_reset(buf);
 	if (!read_oneliner(buf, rebase_path_strategy(), 0))
 		return;
+	free(opts->strategy);
 	opts->strategy = strbuf_detach(buf, NULL);
 	if (!read_oneliner(buf, rebase_path_strategy_opts(), 0))
 		return;
-- 
2.38.0.1467.g709fbdff1a9

[PATCH v2 11/15] connected.c: free the "struct packed_git"

[Ævar Arnfjörð Bjarmason]

The "new_pack" we allocate in check_connected() wasn't being
free'd. Let's do that before we return from the function. This has
leaked ever since "new_pack" was added to this function in
c6807a40dcd (clone: open a shortcut for connectivity check,
2013-05-26).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 connected.c                         | 6 +++++-
 t/t3050-subprojects-fetch.sh        | 1 +
 t/t4067-diff-partial-clone.sh       | 1 +
 t/t5544-pack-objects-hook.sh        | 2 ++
 t/t5610-clone-detached.sh           | 1 +
 t/t5611-clone-config.sh             | 1 +
 t/t5614-clone-submodules-shallow.sh | 1 +
 t/t5617-clone-submodules-remote.sh  | 1 +
 8 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/connected.c b/connected.c
index 74a20cb32e7..b7770825c7b 100644
--- a/connected.c
+++ b/connected.c
@@ -85,6 +85,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
 promisor_pack_found:
 			;
 		} while ((oid = fn(cb_data)) != NULL);
+		free(new_pack);
 		return 0;
 	}
 
@@ -118,8 +119,10 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
 	else
 		rev_list.no_stderr = opt->quiet;
 
-	if (start_command(&rev_list))
+	if (start_command(&rev_list)) {
+		free(new_pack);
 		return error(_("Could not run 'git rev-list'"));
+	}
 
 	sigchain_push(SIGPIPE, SIG_IGN);
 
@@ -151,5 +154,6 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
 		err = error_errno(_("failed to close rev-list's stdin"));
 
 	sigchain_pop(SIGPIPE);
+	free(new_pack);
 	return finish_command(&rev_list) || err;
 }
diff --git a/t/t3050-subprojects-fetch.sh b/t/t3050-subprojects-fetch.sh
index f1f09abdd9b..38846941655 100755
--- a/t/t3050-subprojects-fetch.sh
+++ b/t/t3050-subprojects-fetch.sh
@@ -2,6 +2,7 @@
 
 test_description='fetching and pushing project with subproject'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '
diff --git a/t/t4067-diff-partial-clone.sh b/t/t4067-diff-partial-clone.sh
index 28f42a4046e..f60f5cbd65f 100755
--- a/t/t4067-diff-partial-clone.sh
+++ b/t/t4067-diff-partial-clone.sh
@@ -2,6 +2,7 @@
 
 test_description='behavior of diff when reading objects in a partial clone'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'git show batches blobs' '
diff --git a/t/t5544-pack-objects-hook.sh b/t/t5544-pack-objects-hook.sh
index 54f54f8d2eb..1a9e14bbccd 100755
--- a/t/t5544-pack-objects-hook.sh
+++ b/t/t5544-pack-objects-hook.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='test custom script in place of pack-objects'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'create some history to fetch' '
diff --git a/t/t5610-clone-detached.sh b/t/t5610-clone-detached.sh
index a7ec21eda5a..022ed3d87c3 100755
--- a/t/t5610-clone-detached.sh
+++ b/t/t5610-clone-detached.sh
@@ -4,6 +4,7 @@ test_description='test cloning a repository with detached HEAD'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 head_is_detached() {
diff --git a/t/t5611-clone-config.sh b/t/t5611-clone-config.sh
index 4b3877216ee..727caff4433 100755
--- a/t/t5611-clone-config.sh
+++ b/t/t5611-clone-config.sh
@@ -4,6 +4,7 @@ test_description='tests for git clone -c key=value'
 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 'clone -c sets config in cloned repo' '
diff --git a/t/t5614-clone-submodules-shallow.sh b/t/t5614-clone-submodules-shallow.sh
index 0c85ef834ab..c2a2bb453ee 100755
--- a/t/t5614-clone-submodules-shallow.sh
+++ b/t/t5614-clone-submodules-shallow.sh
@@ -2,6 +2,7 @@
 
 test_description='Test shallow cloning of repos with submodules'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 pwd=$(pwd)
diff --git a/t/t5617-clone-submodules-remote.sh b/t/t5617-clone-submodules-remote.sh
index 68843382493..5a4d7936a72 100755
--- a/t/t5617-clone-submodules-remote.sh
+++ b/t/t5617-clone-submodules-remote.sh
@@ -5,6 +5,7 @@ test_description='Test cloning repos with submodules using remote-tracking branc
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 pwd=$(pwd)
-- 
2.38.0.1467.g709fbdff1a9

[PATCH v2 12/15] rebase: don't leak on "--abort"

[Ævar Arnfjörð Bjarmason]

Fix a leak in the recent 6159e7add49 (rebase --abort: improve reflog
message, 2022-10-12). Before that commit we'd strbuf_release() the
reflog message we were formatting, but when that code was refactored
to use "ropts.head_msg" the strbuf_release() was omitted.

Ideally the three users of "ropts" in cmd_rebase() should use
different "ropts" variables, in practice they're completely separate,
as this and the other user in the "switch" statement will "goto
cleanup", which won't touch "ropts".

The third caller after the "switch" is then unreachable if we take
these two branches, so all of them are getting a "{ 0 }" init'd
"ropts".

So it's OK that we're leaving a stale pointer in "ropts.head_msg",
cleaning it up was our responsibility, and it won't be used again.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/rebase.c          | 1 +
 t/t7517-per-repo-email.sh | 1 +
 2 files changed, 2 insertions(+)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 6a831320313..3f360eb2f37 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1322,6 +1322,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		if (reset_head(the_repository, &ropts) < 0)
 			die(_("could not move back to %s"),
 			    oid_to_hex(&options.orig_head->object.oid));
+		strbuf_release(&head_msg);
 		remove_branch_state(the_repository, 0);
 		ret = finish_rebase(&options);
 		goto cleanup;
diff --git a/t/t7517-per-repo-email.sh b/t/t7517-per-repo-email.sh
index 163ae804685..efc6496e2b2 100755
--- a/t/t7517-per-repo-email.sh
+++ b/t/t7517-per-repo-email.sh
@@ -9,6 +9,7 @@ test_description='per-repo forced setting of email address'
 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 a likely user.useConfigOnly use case' '
-- 
2.38.0.1467.g709fbdff1a9

[PATCH v2 13/15] cherry-pick: free "struct replay_opts" members

[Ævar Arnfjörð Bjarmason]

Call the release_revisions() function added in
1878b5edc03 (revision.[ch]: provide and start using a
release_revisions(), 2022-04-13) in cmd_cherry_pick(), as well as
freeing the xmalloc()'d "revs" member itself.

This is the same change as the one made for cmd_revert() a few lines
above it in fd74ac95ac3 (revert: free "struct replay_opts" members,
2022-07-01).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/revert.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/builtin/revert.c b/builtin/revert.c
index ee32c714a76..0f81c8a795a 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -261,6 +261,9 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 	opts.action = REPLAY_PICK;
 	sequencer_init_config(&opts);
 	res = run_sequencer(argc, argv, &opts);
+	if (opts.revs)
+		release_revisions(opts.revs);
+	free(opts.revs);
 	if (res < 0)
 		die(_("cherry-pick failed"));
 	return res;
-- 
2.38.0.1467.g709fbdff1a9

[PATCH v2 14/15] revert: fix parse_options_concat() leak

[Ævar Arnfjörð Bjarmason]

Free memory from parse_options_concat(), which comes from code
originally added (then extended) in [1].

At this point we could get several more tests leak-free by free()-ing
the xstrdup() just above the line being changed, but that one's
trickier than it seems. The sequencer_remove_state() function
supposedly owns it, but sometimes we don't call it. I have a fix for
it, but it's non-trivial, so let's fix the easy one first.

1. c62f6ec341b (revert: add --ff option to allow fast forward when
   cherry-picking, 2010-03-06)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/revert.c            | 1 +
 t/t3429-rebase-edit-todo.sh | 1 +
 2 files changed, 2 insertions(+)

diff --git a/builtin/revert.c b/builtin/revert.c
index 0f81c8a795a..8bc87e4c770 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -221,6 +221,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
 	opts->strategy = xstrdup_or_null(opts->strategy);
 	if (!opts->strategy && getenv("GIT_TEST_MERGE_ALGORITHM"))
 		opts->strategy = xstrdup(getenv("GIT_TEST_MERGE_ALGORITHM"));
+	free(options);
 
 	if (cmd == 'q') {
 		int ret = sequencer_remove_state(opts);
diff --git a/t/t3429-rebase-edit-todo.sh b/t/t3429-rebase-edit-todo.sh
index abd66f36021..8e0d03969a2 100755
--- a/t/t3429-rebase-edit-todo.sh
+++ b/t/t3429-rebase-edit-todo.sh
@@ -2,6 +2,7 @@
 
 test_description='rebase should reread the todo file if an exec modifies it'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-rebase.sh
 
-- 
2.38.0.1467.g709fbdff1a9

[PATCH v2 15/15] built-ins: use free() not UNLEAK() if trivial, rm dead code

[Ævar Arnfjörð Bjarmason]

For a lot of uses of UNLEAK() it would be quite tricky to release the
memory involved, or we're missing the relevant *_(release|clear)()
functions. But in these cases we have them already, and can just
invoke them on the variable(s) involved, instead of UNLEAK().

For "builtin/worktree.c" the UNLEAK() was also added in [1], but the
struct member it's unleaking was removed in [2]. The only non-"int"
member of that structure is "const char *keep_locked", which comes to
us via "argv" or a string literal[3].

We have good visibility via the compiler and
tooling (e.g. SANITIZE=address) on bad free()-ing, but none on
UNLEAK() we don't need anymore. So let's prefer releasing the memory
when it's easy.

For "bugreport", "worktree" and "config" we need to start using a "ret
= ..." return pattern. For "builtin/bugreport.c" these UNLEAK() were
added in [4], and for "builtin/config.c" in [1].

For "config" the code seen here was the only user of the "value"
variable. For "ACTION_{RENAME,REMOVE}_SECTION" we need to be sure to
return the right exit code in the cases where we were relying on
falling through to the top-level.

I think there's still a use-case for UNLEAK(), but hat it's changed
since then. Using it so that "we can see the real leaks" is
counter-productive in these cases.

It's more useful to have UNLEAK() be a marker of the remaining odd
cases where it's hard to free() the memory for whatever reason. With
this change less than 20 of them remain in-tree.

1. 0e5bba53af7 (add UNLEAK annotation for reducing leak false
   positives, 2017-09-08)
2. d861d34a6ed (worktree: remove extra members from struct add_opts,
   2018-04-24)
3. 0db4961c49b (worktree: teach `add` to accept --reason <string> with
  --lock, 2021-07-15)
4. 0e5bba53af7 and 00d8c311050 (commit: fix "author_ident" leak,
   2022-05-12).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/add.c       |  2 +-
 builtin/bugreport.c |  9 ++++++---
 builtin/commit.c    |  6 +++---
 builtin/config.c    | 42 ++++++++++++++++++++----------------------
 builtin/diff.c      |  2 +-
 builtin/worktree.c  |  7 ++++---
 6 files changed, 35 insertions(+), 33 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index f84372964c8..c68ebafed5e 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -695,6 +695,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		die(_("Unable to write new index file"));
 
 	dir_clear(&dir);
-	UNLEAK(pathspec);
+	clear_pathspec(&pathspec);
 	return exit_status;
 }
diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index 96052541cbf..5bc254be80f 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -106,6 +106,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
 	const char *user_relative_path = NULL;
 	char *prefixed_filename;
 	size_t output_path_len;
+	int ret;
 
 	const struct option bugreport_options[] = {
 		OPT_CALLBACK_F(0, "diagnose", &diagnose, N_("mode"),
@@ -182,7 +183,9 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
 		user_relative_path);
 
 	free(prefixed_filename);
-	UNLEAK(buffer);
-	UNLEAK(report_path);
-	return !!launch_editor(report_path.buf, NULL, NULL);
+	strbuf_release(&buffer);
+
+	ret = !!launch_editor(report_path.buf, NULL, NULL);
+	strbuf_release(&report_path);
+	return ret;
 }
diff --git a/builtin/commit.c b/builtin/commit.c
index c291199b704..f88a29167f4 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1874,8 +1874,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	apply_autostash(git_path_merge_autostash(the_repository));
 
 cleanup:
-	UNLEAK(author_ident);
-	UNLEAK(err);
-	UNLEAK(sb);
+	strbuf_release(&author_ident);
+	strbuf_release(&err);
+	strbuf_release(&sb);
 	return ret;
 }
diff --git a/builtin/config.c b/builtin/config.c
index 753e5fac297..060cf9f3e05 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -639,8 +639,9 @@ static char *default_user_config(void)
 int cmd_config(int argc, const char **argv, const char *prefix)
 {
 	int nongit = !startup_info->have_repository;
-	char *value;
+	char *value = NULL;
 	int flags = 0;
+	int ret = 0;
 
 	given_config_source.file = xstrdup_or_null(getenv(CONFIG_ENVIRONMENT));
 
@@ -856,44 +857,38 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		free(config_file);
 	}
 	else if (actions == ACTION_SET) {
-		int ret;
 		check_write();
 		check_argc(argc, 2, 2);
 		value = normalize_value(argv[0], argv[1]);
-		UNLEAK(value);
 		ret = git_config_set_in_file_gently(given_config_source.file, argv[0], value);
 		if (ret == CONFIG_NOTHING_SET)
 			error(_("cannot overwrite multiple values with a single value\n"
 			"       Use a regexp, --add or --replace-all to change %s."), argv[0]);
-		return ret;
 	}
 	else if (actions == ACTION_SET_ALL) {
 		check_write();
 		check_argc(argc, 2, 3);
 		value = normalize_value(argv[0], argv[1]);
-		UNLEAK(value);
-		return git_config_set_multivar_in_file_gently(given_config_source.file,
-							      argv[0], value, argv[2],
-							      flags);
+		ret = git_config_set_multivar_in_file_gently(given_config_source.file,
+							     argv[0], value, argv[2],
+							     flags);
 	}
 	else if (actions == ACTION_ADD) {
 		check_write();
 		check_argc(argc, 2, 2);
 		value = normalize_value(argv[0], argv[1]);
-		UNLEAK(value);
-		return git_config_set_multivar_in_file_gently(given_config_source.file,
-							      argv[0], value,
-							      CONFIG_REGEX_NONE,
-							      flags);
+		ret = git_config_set_multivar_in_file_gently(given_config_source.file,
+							     argv[0], value,
+							     CONFIG_REGEX_NONE,
+							     flags);
 	}
 	else if (actions == ACTION_REPLACE_ALL) {
 		check_write();
 		check_argc(argc, 2, 3);
 		value = normalize_value(argv[0], argv[1]);
-		UNLEAK(value);
-		return git_config_set_multivar_in_file_gently(given_config_source.file,
-							      argv[0], value, argv[2],
-							      flags | CONFIG_FLAGS_MULTI_REPLACE);
+		ret = git_config_set_multivar_in_file_gently(given_config_source.file,
+							     argv[0], value, argv[2],
+							     flags | CONFIG_FLAGS_MULTI_REPLACE);
 	}
 	else if (actions == ACTION_GET) {
 		check_argc(argc, 1, 2);
@@ -934,26 +929,28 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 							      flags | CONFIG_FLAGS_MULTI_REPLACE);
 	}
 	else if (actions == ACTION_RENAME_SECTION) {
-		int ret;
 		check_write();
 		check_argc(argc, 2, 2);
 		ret = git_config_rename_section_in_file(given_config_source.file,
 							argv[0], argv[1]);
 		if (ret < 0)
 			return ret;
-		if (ret == 0)
+		else if (!ret)
 			die(_("no such section: %s"), argv[0]);
+		else
+			ret = 0;
 	}
 	else if (actions == ACTION_REMOVE_SECTION) {
-		int ret;
 		check_write();
 		check_argc(argc, 1, 1);
 		ret = git_config_rename_section_in_file(given_config_source.file,
 							argv[0], NULL);
 		if (ret < 0)
 			return ret;
-		if (ret == 0)
+		else if (!ret)
 			die(_("no such section: %s"), argv[0]);
+		else
+			ret = 0;
 	}
 	else if (actions == ACTION_GET_COLOR) {
 		check_argc(argc, 1, 2);
@@ -966,5 +963,6 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		return get_colorbool(argv[0], argc == 2);
 	}
 
-	return 0;
+	free(value);
+	return ret;
 }
diff --git a/builtin/diff.c b/builtin/diff.c
index 854d2c5a5c4..cb63f157dd1 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -609,7 +609,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	if (1 < rev.diffopt.skip_stat_unmatch)
 		refresh_index_quietly();
 	release_revisions(&rev);
-	UNLEAK(ent);
+	object_array_clear(&ent);
 	UNLEAK(blob);
 	return result;
 }
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 4a24d53be15..591d659faea 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -629,6 +629,7 @@ static int add(int ac, const char **av, const char *prefix)
 			 N_("try to match the new branch name with a remote-tracking branch")),
 		OPT_END()
 	};
+	int ret;
 
 	memset(&opts, 0, sizeof(opts));
 	opts.checkout = 1;
@@ -705,9 +706,9 @@ static int add(int ac, const char **av, const char *prefix)
 		die(_("--[no-]track can only be used if a new branch is created"));
 	}
 
-	UNLEAK(path);
-	UNLEAK(opts);
-	return add_worktree(path, branch, &opts);
+	ret = add_worktree(path, branch, &opts);
+	free(path);
+	return ret;
 }
 
 static void show_worktree_porcelain(struct worktree *wt, int line_terminator)
-- 
2.38.0.1467.g709fbdff1a9

Re: [PATCH v2 00/15] leak fixes: use existing constructors & other trivia

[Taylor Blau]

On Tue, Nov 08, 2022 at 07:17:36PM +0100, Ævar Arnfjörð Bjarmason wrote:
> This is a bag of simple leak fixes, mostly one-line additions where we
> use existing destructors. See [1] for v1.
>
> Changes since v1:
>
>  * Rebased on pw/rebase-no-reflog-action, per [2].
>  * It'll merge just fine without pw/rebase-no-reflog-action, but some
>    of the tests marked as leak-free will still leak then. So, in
>    practice they can be staged independently, as long as
>    pw/rebase-no-reflog-action is merged down first.
>  * The 10/15 here has an updated explanation about why we were leaking
>    that value. see also[3].

Looking good. I did a --no-ff merge of 'pw/rebase-no-reflog-action' into
this topic, and pushed out the result.

Let's start merging it down.

Thanks,
Taylor