threads / patch / 56655

patch, 7 partstests: fix a memory leak in test-prio-queue.c

Subject: [PATCH 1/7] tests: fix a memory leak in test-prio-queue.c

## tl;dr

23 messages between Oct 6, 2021 and Oct 14, 2021. Diffs are folded; open one to read it.

replies: 22people: 3as markdown or json

Ævar Arnfjörð Bjarmason· Oct 6, 2021, 10:02 UTC · lore

[PATCH 0/7] leak tests: fix "test-tool" & other small leaks

Like my just-submitted series to mark existing tests as passing[1] under the test mode added in ab/sanitize-leak-ci, this marks more tests as passing, but here we need to fix some small memory leaks. This goes on top of ab/sanitize-leak-ci.

Like [1] I merged each of these with "seen" and tested them with GIT_TEST_PASSING_SANITIZE_LEAK=true, so they should hopefully not be a hassle while cooking. This doesn't inter-depend on any other topic I have except ab/sanitize-leak-ci.

But with the outstanding topics I've got in this area (+ [2] + [3] + [4]) and Elijah's en/removing-untracked-fixes these topics in combination will get us to a spot where we can start fixing the big blocking memory leaks in the test suite.

I.e. most tests fail because "git log" and "git checkout" leak when doing almost anything. I've got patches on top of this which fix both of those for 80-90% of cases. After that most failing tests will have failures because of things specific to those tests, not just because their setup code dies as they use "git checkout" or "git log" to set something up.

1. https://lore.kernel.org/git/cover-00.10-00000000000-20211006T094705Z-avarab@gmail.com/
2. https://lore.kernel.org/git/cover-0.2-00000000000-20211006T093405Z-avarab@gmail.com
3. https://lore.kernel.org/git/cover-v4-0.5-00000000000-20211002T201434Z-avarab@gmail.com/
4. https://lore.kernel.org/git/cover-v2-0.5-00000000000-20210927T124407Z-avarab@gmail.com/#t

Ævar Arnfjörð Bjarmason (7):
  tests: fix a memory leak in test-prio-queue.c
  tests: fix a memory leak in test-parse-options.c
  tests: fix a memory leak in test-oidtree.c
  tests: fix test-oid-array leak, test in SANITIZE=leak
  ls-files: fix a trivial dir_clear() leak
  ls-files: add missing string_list_clear()
  merge: add missing strbuf_release()

 builtin/ls-files.c                 | 14 ++++++--------
 builtin/merge.c                    |  2 ++
 t/helper/test-oid-array.c          |  4 ++++
 t/helper/test-oidtree.c            |  3 +++
 t/helper/test-parse-options.c      |  7 ++++++-
 t/helper/test-prio-queue.c         |  2 ++
 t/t0009-prio-queue.sh              |  2 ++
 t/t0040-parse-options.sh           |  1 +
 t/t0064-oid-array.sh               |  2 ++
 t/t0069-oidtree.sh                 |  1 +
 t/t3001-ls-files-others-exclude.sh |  5 +++--
 t/t3005-ls-files-relative.sh       |  1 +
 t/t3020-ls-files-error-unmatch.sh  |  2 ++
 t/t3700-add.sh                     |  1 +
 t/t7104-reset-hard.sh              |  1 +
 t/t7604-merge-custom-message.sh    |  1 +
 16 files changed, 38 insertions(+), 11 deletions(-)

-- 
2.33.0.1441.gbbcdb4c3c66

Ævar Arnfjörð Bjarmason· Oct 6, 2021, 10:02 UTC · re: Ævar Arnfjörð Bjarmason · lore

Fix a memory leak in t/helper/test-prio-queue.c, the lack of freeing the memory with clear_prio_queue() has been there ever since this code was originally added in b4b594a3154 (prio-queue: priority queue of pointers to structs, 2013-06-06).

By fixing this leak we can cleanly run t0009-prio-queue.sh under SANITIZE=leak, so annotate it as such with TEST_PASSES_SANITIZE_LEAK=true.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/helper/test-prio-queue.c | 2 ++
 t/t0009-prio-queue.sh      | 2 ++
 2 files changed, 4 insertions(+)

Show changes to 2 files +4 −0

t/helper/test-prio-queue.c, t/t0009-prio-queue.sh

diff --git a/t/helper/test-prio-queue.c b/t/helper/test-prio-queue.c
index f4028442e37..133b5e6f4ae 100644
--- a/t/helper/test-prio-queue.c
+++ b/t/helper/test-prio-queue.c
@@ -46,5 +46,7 @@ int cmd__prio_queue(int argc, const char **argv)
 		}
 	}
 
+	clear_prio_queue(&pq);
+
 	return 0;
 }
diff --git a/t/t0009-prio-queue.sh b/t/t0009-prio-queue.sh
index 3941ad25286..eea99107a48 100755
--- a/t/t0009-prio-queue.sh
+++ b/t/t0009-prio-queue.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='basic tests for priority queue implementation'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 cat >expect <<'EOF'

-- 
2.33.0.1441.gbbcdb4c3c66

Ævar Arnfjörð Bjarmason· Oct 6, 2021, 10:02 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 2/7] tests: fix a memory leak in test-parse-options.c

Fix a memory leak in t/helper/test-parse-options.c, we were not freeing the allocated "struct string_list" or its items. While I'm at it move the declaration of the "list" string_list the cmd__parse_options() function.

In c8ba1639165 (parse-options: add OPT_STRING_LIST helper, 2011-06-09) the "list" variable was added, and later on in c8ba1639165 (parse-options: add OPT_STRING_LIST helper, 2011-06-09) the "expect" was added.

The "list" variable was last touched in 2721ce21e43 (use string_list initializer consistently, 2016-06-13), but it was still left at the static scope, it's better to move it to the function for consistency.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/helper/test-parse-options.c | 7 ++++++-
 t/t0040-parse-options.sh      | 1 +
 2 files changed, 7 insertions(+), 1 deletion(-)

Show changes to 2 files +7 −1

t/helper/test-parse-options.c, t/t0040-parse-options.sh

diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c
index a282b6ff13e..48d3cf6692d 100644
--- a/t/helper/test-parse-options.c
+++ b/t/helper/test-parse-options.c
@@ -14,7 +14,6 @@ static int dry_run = 0, quiet = 0;
 static char *string = NULL;
 static char *file = NULL;
 static int ambiguous;
-static struct string_list list = STRING_LIST_INIT_NODUP;
 
 static struct {
 	int called;
@@ -107,6 +106,8 @@ int cmd__parse_options(int argc, const char **argv)
 		NULL
 	};
 	struct string_list expect = STRING_LIST_INIT_NODUP;
+	struct string_list list = STRING_LIST_INIT_NODUP;
+
 	struct option options[] = {
 		OPT_BOOL(0, "yes", &boolean, "get a boolean"),
 		OPT_BOOL('D', "no-doubt", &boolean, "begins with 'no-'"),
@@ -185,5 +186,9 @@ int cmd__parse_options(int argc, const char **argv)
 	for (i = 0; i < argc; i++)
 		show(&expect, &ret, "arg %02d: %s", i, argv[i]);
 
+	expect.strdup_strings = 1;
+	string_list_clear(&expect, 0);
+	string_list_clear(&list, 0);
+
 	return ret;
 }
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index da310ed29b1..ed422a1a616 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -5,6 +5,7 @@
 
 test_description='our own option parser'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 cat >expect <<\EOF

-- 
2.33.0.1441.gbbcdb4c3c66

Elijah Newren· Oct 6, 2021, 16:37 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 2/7] tests: fix a memory leak in test-parse-options.c

On Wed, Oct 6, 2021 at 3:02 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 5 quoted lines

>
> Fix a memory leak in t/helper/test-parse-options.c, we were not
> freeing the allocated "struct string_list" or its items. While I'm at
> it move the declaration of the "list" string_list the
> cmd__parse_options() function.

missing verb after the final "string_list"? I'm having trouble parsing this paragraph.

Show 60 quoted lines

> In c8ba1639165 (parse-options: add OPT_STRING_LIST helper, 2011-06-09)
> the "list" variable was added, and later on in
> c8ba1639165 (parse-options: add OPT_STRING_LIST helper, 2011-06-09)
> the "expect" was added.
>
> The "list" variable was last touched in 2721ce21e43 (use string_list
> initializer consistently, 2016-06-13), but it was still left at the
> static scope, it's better to move it to the function for consistency.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  t/helper/test-parse-options.c | 7 ++++++-
>  t/t0040-parse-options.sh      | 1 +
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c
> index a282b6ff13e..48d3cf6692d 100644
> --- a/t/helper/test-parse-options.c
> +++ b/t/helper/test-parse-options.c
> @@ -14,7 +14,6 @@ static int dry_run = 0, quiet = 0;
>  static char *string = NULL;
>  static char *file = NULL;
>  static int ambiguous;
> -static struct string_list list = STRING_LIST_INIT_NODUP;
>
>  static struct {
>         int called;
> @@ -107,6 +106,8 @@ int cmd__parse_options(int argc, const char **argv)
>                 NULL
>         };
>         struct string_list expect = STRING_LIST_INIT_NODUP;
> +       struct string_list list = STRING_LIST_INIT_NODUP;
> +
>         struct option options[] = {
>                 OPT_BOOL(0, "yes", &boolean, "get a boolean"),
>                 OPT_BOOL('D', "no-doubt", &boolean, "begins with 'no-'"),
> @@ -185,5 +186,9 @@ int cmd__parse_options(int argc, const char **argv)
>         for (i = 0; i < argc; i++)
>                 show(&expect, &ret, "arg %02d: %s", i, argv[i]);
>
> +       expect.strdup_strings = 1;
> +       string_list_clear(&expect, 0);
> +       string_list_clear(&list, 0);
> +
>         return ret;
>  }
> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
> index da310ed29b1..ed422a1a616 100755
> --- a/t/t0040-parse-options.sh
> +++ b/t/t0040-parse-options.sh
> @@ -5,6 +5,7 @@
>
>  test_description='our own option parser'
>
> +TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>
>  cat >expect <<\EOF
> --
> 2.33.0.1441.gbbcdb4c3c66

Otherwise, looks good.

Ævar Arnfjörð Bjarmason· Oct 6, 2021, 10:02 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 3/7] tests: fix a memory leak in test-oidtree.c

Fix a memory leak in t/helper/test-oidtree.c, we were not freeing the "struct strbuf" we used for the stdin input we parsed. This leak has been here ever since 92d8ed8ac10 (oidtree: a crit-bit tree for odb_loose_cache, 2021-07-07).

Now that it's fixed we can declare that t0069-oidtree.sh will pass under GIT_TEST_PASSING_SANITIZE_LEAK=true.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/helper/test-oidtree.c | 3 +++
 t/t0069-oidtree.sh      | 1 +
 2 files changed, 4 insertions(+)

Show changes to 2 files +4 −0

t/helper/test-oidtree.c, t/t0069-oidtree.sh

diff --git a/t/helper/test-oidtree.c b/t/helper/test-oidtree.c
index 180ee28dd93..d48a409f4e4 100644
--- a/t/helper/test-oidtree.c
+++ b/t/helper/test-oidtree.c
@@ -45,5 +45,8 @@ int cmd__oidtree(int argc, const char **argv)
 			die("unknown command: %s", line.buf);
 		}
 	}
+
+	strbuf_release(&line);
+
 	return 0;
 }
diff --git a/t/t0069-oidtree.sh b/t/t0069-oidtree.sh
index bfb1397d7b2..74cc59bf8a7 100755
--- a/t/t0069-oidtree.sh
+++ b/t/t0069-oidtree.sh
@@ -1,6 +1,7 @@
 #!/bin/sh
 
 test_description='basic tests for the oidtree implementation'
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 maxhexsz=$(test_oid hexsz)

-- 
2.33.0.1441.gbbcdb4c3c66

Ævar Arnfjörð Bjarmason· Oct 6, 2021, 10:02 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 5/7] ls-files: fix a trivial dir_clear() leak

Fix an edge case that was missed when the dir_clear() call was added in eceba532141 (dir: fix problematic API to avoid memory leaks, 2020-08-18), we need to also clean up when we're about to exit with non-zero.

That commit says, on the topic of the dir_clear() API and UNLEAK():

    [...]two of them clearly thought about leaks since they had an
    UNLEAK(dir) directive, which to me suggests that the method to
    free the data was too unclear.

I think that 0e5bba53af7 (add UNLEAK annotation for reducing leak false positives, 2017-09-08) which added the UNLEAK() makes it clear that that wasn't the case, rather it was the desire to avoid the complexity of freeing the memory at the end of the program.

This does add a bit of complexity, but I think it's worth it to just fix these leaks when it's easy in built-ins. It allows them to serve as canaries for underlying APIs that shouldn't be leaking, it encourages us to make those freeing APIs nicer for all their users, and it prevents other leaking regressions by being able to mark the entire test as TEST_PASSES_SANITIZE_LEAK=true.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/ls-files.c                | 13 +++++--------
 t/t3005-ls-files-relative.sh      |  1 +
 t/t3020-ls-files-error-unmatch.sh |  2 ++
 t/t3700-add.sh                    |  1 +
 t/t7104-reset-hard.sh             |  1 +
 5 files changed, 10 insertions(+), 8 deletions(-)

Show changes to 5 files +10 −8

builtin/ls-files.c, t/t3005-ls-files-relative.sh, t/t3020-ls-files-error-unmatch.sh, t/t3700-add.sh, t/t7104-reset-hard.sh

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index e6d415e077a..0fecfa3b0c1 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -674,6 +674,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 			 N_("suppress duplicate entries")),
 		OPT_END()
 	};
+	int ret = 0;
 
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage_with_options(ls_files_usage, builtin_ls_files_options);
@@ -777,16 +778,12 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 	if (show_resolve_undo)
 		show_ru_info(the_repository->index);
 
-	if (ps_matched) {
-		int bad;
-		bad = report_path_error(ps_matched, &pathspec);
-		if (bad)
-			fprintf(stderr, "Did you forget to 'git add'?\n");
-
-		return bad ? 1 : 0;
+	if (ps_matched && report_path_error(ps_matched, &pathspec)) {
+		fprintf(stderr, "Did you forget to 'git add'?\n");
+		ret = 1;
 	}
 
 	dir_clear(&dir);
 	free(max_prefix);
-	return 0;
+	return ret;
 }
diff --git a/t/t3005-ls-files-relative.sh b/t/t3005-ls-files-relative.sh
index 727e9ae1a44..6ba8b589cd0 100755
--- a/t/t3005-ls-files-relative.sh
+++ b/t/t3005-ls-files-relative.sh
@@ -5,6 +5,7 @@ test_description='ls-files tests with relative paths
 This test runs git ls-files with various relative path arguments.
 '
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'prepare' '
diff --git a/t/t3020-ls-files-error-unmatch.sh b/t/t3020-ls-files-error-unmatch.sh
index 124e73b8e60..2cbcbc0721b 100755
--- a/t/t3020-ls-files-error-unmatch.sh
+++ b/t/t3020-ls-files-error-unmatch.sh
@@ -9,6 +9,8 @@ This test runs git ls-files --error-unmatch to ensure it correctly
 returns an error when a non-existent path is provided on the command
 line.
 '
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 4086e1ebbc9..283a66955d6 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -5,6 +5,7 @@
 
 test_description='Test of git add, including the -- option.'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Test the file mode "$1" of the file "$2" in the index.
diff --git a/t/t7104-reset-hard.sh b/t/t7104-reset-hard.sh
index 7948ec392b3..cf9697eba9a 100755
--- a/t/t7104-reset-hard.sh
+++ b/t/t7104-reset-hard.sh
@@ -2,6 +2,7 @@
 
 test_description='reset --hard unmerged'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '

-- 
2.33.0.1441.gbbcdb4c3c66

Ævar Arnfjörð Bjarmason· Oct 6, 2021, 10:02 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 4/7] tests: fix test-oid-array leak, test in SANITIZE=leak

Fix a trivial memory leak present ever since 38d905bf585 (sha1-array: add test-sha1-array and basic tests, 2014-10-01), now that that's fixed we can test this under GIT_TEST_PASSING_SANITIZE_LEAK=true.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/helper/test-oid-array.c | 4 ++++
 t/t0064-oid-array.sh      | 2 ++
 2 files changed, 6 insertions(+)

Show changes to 2 files +6 −0

t/helper/test-oid-array.c, t/t0064-oid-array.sh

diff --git a/t/helper/test-oid-array.c b/t/helper/test-oid-array.c
index b16cd0b11b1..d1324d086a2 100644
--- a/t/helper/test-oid-array.c
+++ b/t/helper/test-oid-array.c
@@ -35,5 +35,9 @@ int cmd__oid_array(int argc, const char **argv)
 		else
 			die("unknown command: %s", line.buf);
 	}
+
+	strbuf_release(&line);
+	oid_array_clear(&array);
+
 	return 0;
 }
diff --git a/t/t0064-oid-array.sh b/t/t0064-oid-array.sh
index 2e5438ccdac..88c89e8f48a 100755
--- a/t/t0064-oid-array.sh
+++ b/t/t0064-oid-array.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='basic tests for the oid array implementation'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 echoid () {

-- 
2.33.0.1441.gbbcdb4c3c66

Ævar Arnfjörð Bjarmason· Oct 6, 2021, 10:02 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 6/7] ls-files: add missing string_list_clear()

Fix a memory leak that's been here ever since 72aeb18772d (clean.c, ls-files.c: respect encapsulation of exclude_list_groups, 2013-01-16), we dup'd the argument in option_parse_exclude(), but never freed the string_list.

This makes almost all of t3001-ls-files-others-exclude.sh pass (it had a lot of failures before). Let's mark it as passing with TEST_PASSES_SANITIZE_LEAK=true, and then exclude the tests that still failed with a !SANITIZE_LEAK prerequisite check until we fix those leaks. We can still see the failed tests under GIT_TEST_FAIL_PREREQS=true.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/ls-files.c                 | 1 +
 t/t3001-ls-files-others-exclude.sh | 5 +++--
 2 files changed, 4 insertions(+), 2 deletions(-)

Show changes to 2 files +4 −2

builtin/ls-files.c, t/t3001-ls-files-others-exclude.sh

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 0fecfa3b0c1..6e5ac0780e4 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -783,6 +783,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 		ret = 1;
 	}
 
+	string_list_clear(&exclude_list, 0);
 	dir_clear(&dir);
 	free(max_prefix);
 	return ret;
diff --git a/t/t3001-ls-files-others-exclude.sh b/t/t3001-ls-files-others-exclude.sh
index 516c95ea0e8..48cec4e5f88 100755
--- a/t/t3001-ls-files-others-exclude.sh
+++ b/t/t3001-ls-files-others-exclude.sh
@@ -8,6 +8,7 @@ test_description='git ls-files --others --exclude
 This test runs git ls-files --others and tests --exclude patterns.
 '
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 rm -fr one three
@@ -102,7 +103,7 @@ test_expect_success \
        >output &&
      test_cmp expect output'
 
-test_expect_success 'restore gitignore' '
+test_expect_success !SANITIZE_LEAK 'restore gitignore' '
 	git checkout --ignore-skip-worktree-bits $allignores &&
 	rm .git/index
 '
@@ -125,7 +126,7 @@ cat > expect << EOF
 #	three/
 EOF
 
-test_expect_success 'git status honors core.excludesfile' \
+test_expect_success !SANITIZE_LEAK 'git status honors core.excludesfile' \
 	'test_cmp expect output'
 
 test_expect_success 'trailing slash in exclude allows directory match(1)' '

-- 
2.33.0.1441.gbbcdb4c3c66

Ævar Arnfjörð Bjarmason· Oct 6, 2021, 10:02 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 7/7] merge: add missing strbuf_release()

We strbuf_reset() this "struct strbuf" in a loop earlier, but never freed it. Plugs a memory leak that's been here ever since this code got introduced in 1c7b76be7d6 (Build in merge, 2008-07-07).

This takes us from 68 failed tests in "t7600-merge.sh" to 59 under SANITIZE=leak, and makes "t7604-merge-custom-message.sh" pass!

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

Show changes to 2 files +3 −0

builtin/merge.c, t/t7604-merge-custom-message.sh

diff --git a/builtin/merge.c b/builtin/merge.c
index b4bdba2faf6..cd0f0b9e3f2 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1579,6 +1579,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 
 		finish(head_commit, remoteheads, &commit->object.oid, msg.buf);
 		remove_merge_branch_state(the_repository);
+		strbuf_release(&msg);
 		goto done;
 	} else if (!remoteheads->next && common->next)
 		;
@@ -1749,6 +1750,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		ret = suggest_conflicts();
 
 done:
+	strbuf_release(&buf);
 	free(branch_to_free);
 	return ret;
 }
diff --git a/t/t7604-merge-custom-message.sh b/t/t7604-merge-custom-message.sh
index cd4f9607dc1..eca75551016 100755
--- a/t/t7604-merge-custom-message.sh
+++ b/t/t7604-merge-custom-message.sh
@@ -4,6 +4,7 @@ test_description='git merge
 
 Testing merge when using a custom message for the merge commit.'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 create_merge_msgs() {

-- 
2.33.0.1441.gbbcdb4c3c66

Elijah Newren· Oct 6, 2021, 16:47 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 0/7] leak tests: fix "test-tool" & other small leaks

On Wed, Oct 6, 2021 at 3:02 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 10 quoted lines

>
> Like my just-submitted series to mark existing tests as passing[1]
> under the test mode added in ab/sanitize-leak-ci, this marks more
> tests as passing, but here we need to fix some small memory
> leaks. This goes on top of ab/sanitize-leak-ci.
>
> Like [1] I merged each of these with "seen" and tested them with
> GIT_TEST_PASSING_SANITIZE_LEAK=true, so they should hopefully not be a
> hassle while cooking. This doesn't inter-depend on any other topic I
> have except ab/sanitize-leak-ci.

Modulo a tiny comment on one of the commit messages, this series looks good to me.

Show 47 quoted lines

>
> But with the outstanding topics I've got in this area (+ [2] + [3] +
> [4]) and Elijah's en/removing-untracked-fixes these topics in
> combination will get us to a spot where we can start fixing the big blocking memory leaks in the test suite.
>
> I.e. most tests fail because "git log" and "git checkout" leak when
> doing almost anything. I've got patches on top of this which fix both
> of those for 80-90% of cases. After that most failing tests will have
> failures because of things specific to those tests, not just because
> their setup code dies as they use "git checkout" or "git log" to set
> something up.
>
> 1. https://lore.kernel.org/git/cover-00.10-00000000000-20211006T094705Z-avarab@gmail.com/
> 2. https://lore.kernel.org/git/cover-0.2-00000000000-20211006T093405Z-avarab@gmail.com
> 3. https://lore.kernel.org/git/cover-v4-0.5-00000000000-20211002T201434Z-avarab@gmail.com/
> 4. https://lore.kernel.org/git/cover-v2-0.5-00000000000-20210927T124407Z-avarab@gmail.com/#t
>
> Ævar Arnfjörð Bjarmason (7):
>   tests: fix a memory leak in test-prio-queue.c
>   tests: fix a memory leak in test-parse-options.c
>   tests: fix a memory leak in test-oidtree.c
>   tests: fix test-oid-array leak, test in SANITIZE=leak
>   ls-files: fix a trivial dir_clear() leak
>   ls-files: add missing string_list_clear()
>   merge: add missing strbuf_release()
>
>  builtin/ls-files.c                 | 14 ++++++--------
>  builtin/merge.c                    |  2 ++
>  t/helper/test-oid-array.c          |  4 ++++
>  t/helper/test-oidtree.c            |  3 +++
>  t/helper/test-parse-options.c      |  7 ++++++-
>  t/helper/test-prio-queue.c         |  2 ++
>  t/t0009-prio-queue.sh              |  2 ++
>  t/t0040-parse-options.sh           |  1 +
>  t/t0064-oid-array.sh               |  2 ++
>  t/t0069-oidtree.sh                 |  1 +
>  t/t3001-ls-files-others-exclude.sh |  5 +++--
>  t/t3005-ls-files-relative.sh       |  1 +
>  t/t3020-ls-files-error-unmatch.sh  |  2 ++
>  t/t3700-add.sh                     |  1 +
>  t/t7104-reset-hard.sh              |  1 +
>  t/t7604-merge-custom-message.sh    |  1 +
>  16 files changed, 38 insertions(+), 11 deletions(-)
>
> --
> 2.33.0.1441.gbbcdb4c3c66
>

Ævar Arnfjörð Bjarmason· Oct 7, 2021, 10:01 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 0/7] leak tests: fix "test-tool" & other small leaks

See v1[1] for a more detailed summary, the only update here is to some bad commit message grammar/phrasing in v1.

1. https://lore.kernel.org/git/cover-0.7-00000000000-20211006T095426Z-avarab@gmail.com/

Ævar Arnfjörð Bjarmason (7):
  tests: fix a memory leak in test-prio-queue.c
  tests: fix a memory leak in test-parse-options.c
  tests: fix a memory leak in test-oidtree.c
  tests: fix test-oid-array leak, test in SANITIZE=leak
  ls-files: fix a trivial dir_clear() leak
  ls-files: add missing string_list_clear()
  merge: add missing strbuf_release()

 builtin/ls-files.c                 | 14 ++++++--------
 builtin/merge.c                    |  2 ++
 t/helper/test-oid-array.c          |  4 ++++
 t/helper/test-oidtree.c            |  3 +++
 t/helper/test-parse-options.c      |  7 ++++++-
 t/helper/test-prio-queue.c         |  2 ++
 t/t0009-prio-queue.sh              |  2 ++
 t/t0040-parse-options.sh           |  1 +
 t/t0064-oid-array.sh               |  2 ++
 t/t0069-oidtree.sh                 |  1 +
 t/t3001-ls-files-others-exclude.sh |  5 +++--
 t/t3005-ls-files-relative.sh       |  1 +
 t/t3020-ls-files-error-unmatch.sh  |  2 ++
 t/t3700-add.sh                     |  1 +
 t/t7104-reset-hard.sh              |  1 +
 t/t7604-merge-custom-message.sh    |  1 +
 16 files changed, 38 insertions(+), 11 deletions(-)

Range-diff against v1:
1:  8806f9cb5e8 = 1:  37cdf0ee348 tests: fix a memory leak in test-prio-queue.c
2:  c24e115aa49 ! 2:  53b0da60804 tests: fix a memory leak in test-parse-options.c
    @@ Commit message
         tests: fix a memory leak in test-parse-options.c
     
         Fix a memory leak in t/helper/test-parse-options.c, we were not
    -    freeing the allocated "struct string_list" or its items. While I'm at
    -    it move the declaration of the "list" string_list the
    -    cmd__parse_options() function.
    +    freeing the allocated "struct string_list" or its items. Let's move
    +    the declaration of the "list" variable into the cmd__parse_options()
    +    and release it at the end.
     
         In c8ba1639165 (parse-options: add OPT_STRING_LIST helper, 2011-06-09)
         the "list" variable was added, and later on in
3:  a216297aba1 = 3:  33a4b9c7c68 tests: fix a memory leak in test-oidtree.c
4:  4aa2a70c67e = 4:  b8ce8d7e972 tests: fix test-oid-array leak, test in SANITIZE=leak
5:  58b5bc67435 = 5:  73cf1018953 ls-files: fix a trivial dir_clear() leak
6:  34749645f74 = 6:  fc10353c0c5 ls-files: add missing string_list_clear()
7:  d7c94fa2851 = 7:  9942c084244 merge: add missing strbuf_release()

-- 
2.33.0.1446.g6af949f83bd

Ævar Arnfjörð Bjarmason· Oct 7, 2021, 10:01 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 1/7] tests: fix a memory leak in test-prio-queue.c

By fixing this leak we can cleanly run t0009-prio-queue.sh under SANITIZE=leak, so annotate it as such with TEST_PASSES_SANITIZE_LEAK=true.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/helper/test-prio-queue.c | 2 ++
 t/t0009-prio-queue.sh      | 2 ++
 2 files changed, 4 insertions(+)

Show changes to 2 files +4 −0

t/helper/test-prio-queue.c, t/t0009-prio-queue.sh

diff --git a/t/helper/test-prio-queue.c b/t/helper/test-prio-queue.c
index f4028442e37..133b5e6f4ae 100644
--- a/t/helper/test-prio-queue.c
+++ b/t/helper/test-prio-queue.c
@@ -46,5 +46,7 @@ int cmd__prio_queue(int argc, const char **argv)
 		}
 	}
 
+	clear_prio_queue(&pq);
+
 	return 0;
 }
diff --git a/t/t0009-prio-queue.sh b/t/t0009-prio-queue.sh
index 3941ad25286..eea99107a48 100755
--- a/t/t0009-prio-queue.sh
+++ b/t/t0009-prio-queue.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='basic tests for priority queue implementation'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 cat >expect <<'EOF'

-- 
2.33.0.1446.g6af949f83bd

Ævar Arnfjörð Bjarmason· Oct 7, 2021, 10:01 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 2/7] tests: fix a memory leak in test-parse-options.c

Fix a memory leak in t/helper/test-parse-options.c, we were not freeing the allocated "struct string_list" or its items. Let's move the declaration of the "list" variable into the cmd__parse_options() and release it at the end.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/helper/test-parse-options.c | 7 ++++++-
 t/t0040-parse-options.sh      | 1 +
 2 files changed, 7 insertions(+), 1 deletion(-)

Show changes to 2 files +7 −1

t/helper/test-parse-options.c, t/t0040-parse-options.sh

diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c
index a282b6ff13e..48d3cf6692d 100644
--- a/t/helper/test-parse-options.c
+++ b/t/helper/test-parse-options.c
@@ -14,7 +14,6 @@ static int dry_run = 0, quiet = 0;
 static char *string = NULL;
 static char *file = NULL;
 static int ambiguous;
-static struct string_list list = STRING_LIST_INIT_NODUP;
 
 static struct {
 	int called;
@@ -107,6 +106,8 @@ int cmd__parse_options(int argc, const char **argv)
 		NULL
 	};
 	struct string_list expect = STRING_LIST_INIT_NODUP;
+	struct string_list list = STRING_LIST_INIT_NODUP;
+
 	struct option options[] = {
 		OPT_BOOL(0, "yes", &boolean, "get a boolean"),
 		OPT_BOOL('D', "no-doubt", &boolean, "begins with 'no-'"),
@@ -185,5 +186,9 @@ int cmd__parse_options(int argc, const char **argv)
 	for (i = 0; i < argc; i++)
 		show(&expect, &ret, "arg %02d: %s", i, argv[i]);
 
+	expect.strdup_strings = 1;
+	string_list_clear(&expect, 0);
+	string_list_clear(&list, 0);
+
 	return ret;
 }
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index da310ed29b1..ed422a1a616 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -5,6 +5,7 @@
 
 test_description='our own option parser'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 cat >expect <<\EOF

-- 
2.33.0.1446.g6af949f83bd

Ævar Arnfjörð Bjarmason· Oct 7, 2021, 10:01 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 3/7] tests: fix a memory leak in test-oidtree.c

Now that it's fixed we can declare that t0069-oidtree.sh will pass under GIT_TEST_PASSING_SANITIZE_LEAK=true.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/helper/test-oidtree.c | 3 +++
 t/t0069-oidtree.sh      | 1 +
 2 files changed, 4 insertions(+)

Show changes to 2 files +4 −0

t/helper/test-oidtree.c, t/t0069-oidtree.sh

diff --git a/t/helper/test-oidtree.c b/t/helper/test-oidtree.c
index 180ee28dd93..d48a409f4e4 100644
--- a/t/helper/test-oidtree.c
+++ b/t/helper/test-oidtree.c
@@ -45,5 +45,8 @@ int cmd__oidtree(int argc, const char **argv)
 			die("unknown command: %s", line.buf);
 		}
 	}
+
+	strbuf_release(&line);
+
 	return 0;
 }
diff --git a/t/t0069-oidtree.sh b/t/t0069-oidtree.sh
index bfb1397d7b2..74cc59bf8a7 100755
--- a/t/t0069-oidtree.sh
+++ b/t/t0069-oidtree.sh
@@ -1,6 +1,7 @@
 #!/bin/sh
 
 test_description='basic tests for the oidtree implementation'
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 maxhexsz=$(test_oid hexsz)

-- 
2.33.0.1446.g6af949f83bd

Ævar Arnfjörð Bjarmason· Oct 7, 2021, 10:01 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 4/7] tests: fix test-oid-array leak, test in SANITIZE=leak

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/helper/test-oid-array.c | 4 ++++
 t/t0064-oid-array.sh      | 2 ++
 2 files changed, 6 insertions(+)

Show changes to 2 files +6 −0

t/helper/test-oid-array.c, t/t0064-oid-array.sh

diff --git a/t/helper/test-oid-array.c b/t/helper/test-oid-array.c
index b16cd0b11b1..d1324d086a2 100644
--- a/t/helper/test-oid-array.c
+++ b/t/helper/test-oid-array.c
@@ -35,5 +35,9 @@ int cmd__oid_array(int argc, const char **argv)
 		else
 			die("unknown command: %s", line.buf);
 	}
+
+	strbuf_release(&line);
+	oid_array_clear(&array);
+
 	return 0;
 }
diff --git a/t/t0064-oid-array.sh b/t/t0064-oid-array.sh
index 2e5438ccdac..88c89e8f48a 100755
--- a/t/t0064-oid-array.sh
+++ b/t/t0064-oid-array.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='basic tests for the oid array implementation'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 echoid () {

-- 
2.33.0.1446.g6af949f83bd

Ævar Arnfjörð Bjarmason· Oct 7, 2021, 10:01 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 7/7] merge: add missing strbuf_release()

We strbuf_reset() this "struct strbuf" in a loop earlier, but never freed it. Plugs a memory leak that's been here ever since this code got introduced in 1c7b76be7d6 (Build in merge, 2008-07-07).

This takes us from 68 failed tests in "t7600-merge.sh" to 59 under SANITIZE=leak, and makes "t7604-merge-custom-message.sh" pass!

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

Show changes to 2 files +3 −0

builtin/merge.c, t/t7604-merge-custom-message.sh

diff --git a/builtin/merge.c b/builtin/merge.c
index 0ccd5e1ac83..84d76780f0d 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1577,6 +1577,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 
 		finish(head_commit, remoteheads, &commit->object.oid, msg.buf);
 		remove_merge_branch_state(the_repository);
+		strbuf_release(&msg);
 		goto done;
 	} else if (!remoteheads->next && common->next)
 		;
@@ -1747,6 +1748,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		ret = suggest_conflicts();
 
 done:
+	strbuf_release(&buf);
 	free(branch_to_free);
 	return ret;
 }
diff --git a/t/t7604-merge-custom-message.sh b/t/t7604-merge-custom-message.sh
index cd4f9607dc1..eca75551016 100755
--- a/t/t7604-merge-custom-message.sh
+++ b/t/t7604-merge-custom-message.sh
@@ -4,6 +4,7 @@ test_description='git merge
 
 Testing merge when using a custom message for the merge commit.'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 create_merge_msgs() {

-- 
2.33.0.1446.g6af949f83bd

Ævar Arnfjörð Bjarmason· Oct 7, 2021, 10:01 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 6/7] ls-files: add missing string_list_clear()

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/ls-files.c                 | 1 +
 t/t3001-ls-files-others-exclude.sh | 5 +++--
 2 files changed, 4 insertions(+), 2 deletions(-)

Show changes to 2 files +4 −2

builtin/ls-files.c, t/t3001-ls-files-others-exclude.sh

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index fcc685947f9..031fef1bcaa 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -781,6 +781,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 		ret = 1;
 	}
 
+	string_list_clear(&exclude_list, 0);
 	dir_clear(&dir);
 	free(max_prefix);
 	return ret;
diff --git a/t/t3001-ls-files-others-exclude.sh b/t/t3001-ls-files-others-exclude.sh
index 516c95ea0e8..48cec4e5f88 100755
--- a/t/t3001-ls-files-others-exclude.sh
+++ b/t/t3001-ls-files-others-exclude.sh
@@ -8,6 +8,7 @@ test_description='git ls-files --others --exclude
 This test runs git ls-files --others and tests --exclude patterns.
 '
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 rm -fr one three
@@ -102,7 +103,7 @@ test_expect_success \
        >output &&
      test_cmp expect output'
 
-test_expect_success 'restore gitignore' '
+test_expect_success !SANITIZE_LEAK 'restore gitignore' '
 	git checkout --ignore-skip-worktree-bits $allignores &&
 	rm .git/index
 '
@@ -125,7 +126,7 @@ cat > expect << EOF
 #	three/
 EOF
 
-test_expect_success 'git status honors core.excludesfile' \
+test_expect_success !SANITIZE_LEAK 'git status honors core.excludesfile' \
 	'test_cmp expect output'
 
 test_expect_success 'trailing slash in exclude allows directory match(1)' '

-- 
2.33.0.1446.g6af949f83bd

Ævar Arnfjörð Bjarmason· Oct 7, 2021, 10:01 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 5/7] ls-files: fix a trivial dir_clear() leak

That commit says, on the topic of the dir_clear() API and UNLEAK():

    [...]two of them clearly thought about leaks since they had an
    UNLEAK(dir) directive, which to me suggests that the method to
    free the data was too unclear.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/ls-files.c                | 13 +++++--------
 t/t3005-ls-files-relative.sh      |  1 +
 t/t3020-ls-files-error-unmatch.sh |  2 ++
 t/t3700-add.sh                    |  1 +
 t/t7104-reset-hard.sh             |  1 +
 5 files changed, 10 insertions(+), 8 deletions(-)

Show changes to 5 files +10 −8

builtin/ls-files.c, t/t3005-ls-files-relative.sh, t/t3020-ls-files-error-unmatch.sh, t/t3700-add.sh, t/t7104-reset-hard.sh

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index a2000ed6bf2..fcc685947f9 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -672,6 +672,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 			 N_("suppress duplicate entries")),
 		OPT_END()
 	};
+	int ret = 0;
 
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage_with_options(ls_files_usage, builtin_ls_files_options);
@@ -775,16 +776,12 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 	if (show_resolve_undo)
 		show_ru_info(the_repository->index);
 
-	if (ps_matched) {
-		int bad;
-		bad = report_path_error(ps_matched, &pathspec);
-		if (bad)
-			fprintf(stderr, "Did you forget to 'git add'?\n");
-
-		return bad ? 1 : 0;
+	if (ps_matched && report_path_error(ps_matched, &pathspec)) {
+		fprintf(stderr, "Did you forget to 'git add'?\n");
+		ret = 1;
 	}
 
 	dir_clear(&dir);
 	free(max_prefix);
-	return 0;
+	return ret;
 }
diff --git a/t/t3005-ls-files-relative.sh b/t/t3005-ls-files-relative.sh
index 727e9ae1a44..6ba8b589cd0 100755
--- a/t/t3005-ls-files-relative.sh
+++ b/t/t3005-ls-files-relative.sh
@@ -5,6 +5,7 @@ test_description='ls-files tests with relative paths
 This test runs git ls-files with various relative path arguments.
 '
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'prepare' '
diff --git a/t/t3020-ls-files-error-unmatch.sh b/t/t3020-ls-files-error-unmatch.sh
index 124e73b8e60..2cbcbc0721b 100755
--- a/t/t3020-ls-files-error-unmatch.sh
+++ b/t/t3020-ls-files-error-unmatch.sh
@@ -9,6 +9,8 @@ This test runs git ls-files --error-unmatch to ensure it correctly
 returns an error when a non-existent path is provided on the command
 line.
 '
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 4086e1ebbc9..283a66955d6 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -5,6 +5,7 @@
 
 test_description='Test of git add, including the -- option.'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Test the file mode "$1" of the file "$2" in the index.
diff --git a/t/t7104-reset-hard.sh b/t/t7104-reset-hard.sh
index 7948ec392b3..cf9697eba9a 100755
--- a/t/t7104-reset-hard.sh
+++ b/t/t7104-reset-hard.sh
@@ -2,6 +2,7 @@
 
 test_description='reset --hard unmerged'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '

-- 
2.33.0.1446.g6af949f83bd

Junio C Hamano· Oct 7, 2021, 22:46 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH v2 5/7] ls-files: fix a trivial dir_clear() leak

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 6 quoted lines

> This does add a bit of complexity, but I think it's worth it to just
> fix these leaks when it's easy in built-ins. It allows them to serve
> as canaries for underlying APIs that shouldn't be leaking, it
> encourages us to make those freeing APIs nicer for all their users,
> and it prevents other leaking regressions by being able to mark the
> entire test as TEST_PASSES_SANITIZE_LEAK=true.

This does more than necessary, though. Introducing "ret", replacing an early return with an assignment to it, and returning "ret" instead of hardcoded 0, would have been the "fix a trivial leak", and the "ah, report_path_error() always returns true" does not belong here.

These things look small, but small things add up.

Ævar Arnfjörð Bjarmason· Oct 13, 2021, 13:39 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 5/7] ls-files: fix a trivial dir_clear() leak

On Thu, Oct 07 2021, Junio C Hamano wrote:

Show 16 quoted lines

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> This does add a bit of complexity, but I think it's worth it to just
>> fix these leaks when it's easy in built-ins. It allows them to serve
>> as canaries for underlying APIs that shouldn't be leaking, it
>> encourages us to make those freeing APIs nicer for all their users,
>> and it prevents other leaking regressions by being able to mark the
>> entire test as TEST_PASSES_SANITIZE_LEAK=true.
>
> This does more than necessary, though.  Introducing "ret", replacing
> an early return with an assignment to it, and returning "ret"
> instead of hardcoded 0, would have been the "fix a trivial leak",
> and the "ah, report_path_error() always returns true" does not
> belong here.
>
> These things look small, but small things add up.

I think you mean that you'd have liked:

Show changes to builtin/ls-files.c +10 −14

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index a2000ed6bf2..5e6b6f2d4a0 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -672,6 +672,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
                         N_("suppress duplicate entries")),
                OPT_END()
        };
+       int ret = 0;
 
        if (argc == 2 && !strcmp(argv[1], "-h"))
                usage_with_options(ls_files_usage, builtin_ls_files_options);
@@ -776,15 +777,13 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
                show_ru_info(the_repository->index);
 
        if (ps_matched) {
-               int bad;
-               bad = report_path_error(ps_matched, &pathspec);
-               if (bad)
+               if (report_path_error(ps_matched, &pathspec)) {
                        fprintf(stderr, "Did you forget to 'git add'?\n");
-
-               return bad ? 1 : 0;
+                       ret = 1;
+               }
        }
 
        dir_clear(&dir);
        free(max_prefix);
-       return 0;
+       return ret;
 }

Instead of this:

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index a2000ed6bf2..fcc685947f9 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -672,6 +672,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
                         N_("suppress duplicate entries")),
                OPT_END()
        };
+       int ret = 0;
 
        if (argc == 2 && !strcmp(argv[1], "-h"))
                usage_with_options(ls_files_usage, builtin_ls_files_options);
@@ -775,16 +776,12 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
        if (show_resolve_undo)
                show_ru_info(the_repository->index);
 
-       if (ps_matched) {
-               int bad;
-               bad = report_path_error(ps_matched, &pathspec);
-               if (bad)
-                       fprintf(stderr, "Did you forget to 'git add'?\n");
-
-               return bad ? 1 : 0;
+       if (ps_matched && report_path_error(ps_matched, &pathspec)) {
+               fprintf(stderr, "Did you forget to 'git add'?\n");
+               ret = 1;
        }
 
        dir_clear(&dir);
        free(max_prefix);
-       return 0;
+       return ret;
 }

Doesn't make much sense, but I can re-roll with it if you feel strongly
about it. I think the current version is ready to be picked up.

Yeah we should avoid refactoring-while-at-it, but in cases where a patch
removes the only reason a nested if/if statement exists, unrolling it
into a single "if" handly seems too much. I think the alternative just
leaves the reader squinting at the diff wondering why we still need that
nesting...

Junio C Hamano· Oct 13, 2021, 19:01 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH v2 5/7] ls-files: fix a trivial dir_clear() leak

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Show 20 quoted lines

> -       if (ps_matched) {
> -               int bad;
> -               bad = report_path_error(ps_matched, &pathspec);
> -               if (bad)
> -                       fprintf(stderr, "Did you forget to 'git add'?\n");
> -
> -               return bad ? 1 : 0;
> +       if (ps_matched && report_path_error(ps_matched, &pathspec)) {
> +               fprintf(stderr, "Did you forget to 'git add'?\n");
> +               ret = 1;
>         }
>  
>         dir_clear(&dir);
>         free(max_prefix);
> -       return 0;
> +       return ret;
>  }
>
> Doesn't make much sense, but I can re-roll with it if you feel strongly
> about it. I think the current version is ready to be picked up.

I do not see where that "doesn't make much sense" comes from. If it does not make sense, I wouldn't have mentioned it.

> Yeah we should avoid refactoring-while-at-it, but in cases where a patch
> removes the only reason a nested if/if statement exists, unrolling it

And I do not quite see what "the only reason" is in this case, or what it has to do with the restructuring, either. Care to either clarify, or fix the patch, or perhaps both?

Thanks.

Ævar Arnfjörð Bjarmason· Oct 14, 2021, 00:15 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 5/7] ls-files: fix a trivial dir_clear() leak

On Wed, Oct 13 2021, Junio C Hamano wrote:

Show 25 quoted lines

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> -       if (ps_matched) {
>> -               int bad;
>> -               bad = report_path_error(ps_matched, &pathspec);
>> -               if (bad)
>> -                       fprintf(stderr, "Did you forget to 'git add'?\n");
>> -
>> -               return bad ? 1 : 0;
>> +       if (ps_matched && report_path_error(ps_matched, &pathspec)) {
>> +               fprintf(stderr, "Did you forget to 'git add'?\n");
>> +               ret = 1;
>>         }
>>  
>>         dir_clear(&dir);
>>         free(max_prefix);
>> -       return 0;
>> +       return ret;
>>  }
>>
>> Doesn't make much sense, but I can re-roll with it if you feel strongly
>> about it. I think the current version is ready to be picked up.
>
> I do not see where that "doesn't make much sense" comes from.  If it
> does not make sense, I wouldn't have mentioned it.

I meant "I don't think that makes much sense".

Show 6 quoted lines

>> Yeah we should avoid refactoring-while-at-it, but in cases where a patch
>> removes the only reason a nested if/if statement exists, unrolling it
>
> And I do not quite see what "the only reason" is in this case, or
> what it has to do with the restructuring, either.  Care to either
> clarify, or fix the patch, or perhaps both?

I mean that we generally don't write code like:

	if (x) {
		if (y) {
			fprintf(...);
			ret = 1;
		}
	}

And instead write:

	if (x && y) {
		fprintf(...);
		ret = 1;
	}

So aside from the specific change here I thought your objection would also apply to e.g. removing braces from a standalone "if" as it's reduced to a one statement body, which we we generally do.

But reading your upthread:

    [...]the "ah, report_path_error() always returns true" does not
    belong here.

I think the objection might be in folding that in particular into the "if" statement?

The point of this change is that if report_path_error() was non-zero we'd skip the freeing before, but shouldn't.

So any change here would have wanted to fall through the "if", but we need to do the fprintf() if report_path_error returns true.

So I don't really get that "always returns true" comment means in this context. It returns true on error, and dealing with that code branch in relation to the freeing is what this change needs to concern itself with.

Junio C Hamano· Oct 14, 2021, 17:38 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH v2 5/7] ls-files: fix a trivial dir_clear() leak

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Show 15 quoted lines

> On Wed, Oct 13 2021, Junio C Hamano wrote:
>
>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>
>>> -       if (ps_matched) {
>>> -               int bad;
>>> -               bad = report_path_error(ps_matched, &pathspec);
>>> -               if (bad)
>>> -                       fprintf(stderr, "Did you forget to 'git add'?\n");
>>> -
>>> -               return bad ? 1 : 0;
>>> +       if (ps_matched && report_path_error(ps_matched, &pathspec)) {
>>> +               fprintf(stderr, "Did you forget to 'git add'?\n");
>>> +               ret = 1;
>>>         }

...

Show 19 quoted lines

> I mean that we generally don't write code like:
>
> 	if (x) {
> 		if (y) {
> 			fprintf(...);
> 			ret = 1;
> 		}
> 	}
>
> And instead write:
>
> 	if (x && y) {
> 		fprintf(...);
> 		ret = 1;
> 	}
>
> So aside from the specific change here I thought your objection would
> also apply to e.g. removing braces from a standalone "if" as it's
> reduced to a one statement body, which we we generally do.

Yes, but in a "plug-leak-only" fix, I would have expected

	if (ps_matched) {
		int bad;
		bad = report_path_error(ps_matched, &pathspec);
		if (bad)
			fprintf(stderr, "Did you forget to 'git add'?\n");

- return bad ? 1 : 0; + ret = bad ? 1 : 0;

Doing anything more than that is "while at it clean-up".